{"thread":{"id":"55726","subject":"[PATCH] xsize_t: avoid implementation defined behavior when len < 0","startedAt":"2021-05-18T15:03:54Z","lastAt":"2021-05-19T01:53:01Z","messageCount":3,"participants":["Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424885","messageId":"YKPXVMchtGbwDuue@google.com","threadId":"55726","inReplyTo":null,"subject":"[PATCH] xsize_t: avoid implementation defined behavior when len < 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-05-18T15:03:48Z","receivedAt":"2021-05-18T15:03:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The xsize_t helper aims to safely convert an off_t to a size_t,\nerroring out when a file offset is too large to fit into a memory\naddress. It does this by using two casts:\n\n\tsize_t size = (size_t) len;\n\tif (len != (off_t) size)\n\t\t... error out ...\n\nOn a platform with sizeof(size_t) < sizeof(off_t), this check is safe\nand correct. The first cast truncates to a size_t by finding the\nremainder modulo SIZE_MAX+1 (see C99 section 6.3.1.3 Signed and\nunsigned integers) and the second promotes to an off_t, meaning the\nresult is true if and only if len is representable as a size_t.\n\nOn other platforms, this two-casts strategy still works well (always\nsucceeds) for len >= 0. But for len < 0, when the first cast succeeds\nand produces SIZE_MAX + 1 + len, the resulting value is too large to\nbe represented as an off_t, so the second cast produces implementation\ndefined behavior. In practice, it is likely to produce a result of\ntrue despite len not being representable as size_t.\n\nSimplify by replacing with a more straightforward check: compare len\nto the relevant bounds and then cast it.\n\nIn practice, this is not likely to come up since typical callers use\nnonnegative len. Still, it's helpful to handle this case to make the\nbehavior easy to reason about.\n\nHistorical note: the original bounds-checking in 46be82dfd0 (xsize_t:\ncheck whether we lose bits, 2010-07-28) did not produce this\nimplementation-defined behavior, though it still did not handle\nnegative offsets. It was not until 73560c793a (git-compat-util.h:\nxsize_t() - avoid -Wsign-compare warnings, 2017-09-21) introduced the\ndouble cast that the implementation-defined behavior was triggered.\n\nSigned-off-by: Jonathan Nieder \n---\nHi,\n\nThis is *not* -rc material; it's just something I noticed and figured\nI would send it before I forget (among other benefits, this helps us\nkick the tires on the release candidate by having patches to work\nwith).\n\nThoughts welcome, as always.\n\nJonathan\n\n git-compat-util.h | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a3..20318a0aac 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -986,11 +986,9 @@ static inline char *xstrdup_or_null(const char *str)\n \n static inline size_t xsize_t(off_t len)\n {\n-\tsize_t size = (size_t) len;\n-\n-\tif (len != (off_t) size)\n+\tif (len < 0 || len > SIZE_MAX)\n \t\tdie(\"Cannot handle files this big\");\n-\treturn size;\n+\treturn (size_t) len;\n }\n \n __attribute__((format (printf, 3, 4)))\n-- \n2.31.1.818.g46aad6cb9e\n\n"},{"id":"424912","messageId":"xmqqy2cbzeqx.fsf@gitster.g","threadId":"55726","inReplyTo":"YKPXVMchtGbwDuue@google.com","subject":"Re: [PATCH] xsize_t: avoid implementation defined behavior when len < 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-19T01:36:22Z","receivedAt":"2021-05-19T01:36:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> This is *not* -rc material; it's just something I noticed and figured\n> I would send it before I forget (among other benefits, this helps us\n> kick the tires on the release candidate by having patches to work\n> with).\n>\n> Thoughts welcome, as always.\n>\n> Jonathan\n>\n> git-compat-util.h | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index a508dbe5a3..20318a0aac 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -986,11 +986,9 @@ static inline char *xstrdup_or_null(const char *str)\n> \n> static inline size_t xsize_t(off_t len)\n> {\n> -\tsize_t size = (size_t) len;\n> -\n> -\tif (len != (off_t) size)\n> +\tif (len < 0 || len > SIZE_MAX)\n> \t\tdie(\"Cannot handle files this big\");\n\nOK, so negative offset or offset that cannot be represented as size_t\nare rejected. That is much easier to read than the original ;-)\n\nSIZE_MAX is associated with size_t so it presumably is an unsigned\nconstant; would it again trigger a sign-compare warning?\n\n> -\treturn size;\n> +\treturn (size_t) len;\n> }\n> \n> __attribute__((format (printf, 3, 4)))\n"},{"id":"424914","messageId":"YKRveGhNqPMjmJs8@google.com","threadId":"55726","inReplyTo":"xmqqy2cbzeqx.fsf@gitster.g","subject":"[PATCH v2] xsize_t: avoid implementation defined behavior when len < 0","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-05-19T01:52:56Z","receivedAt":"2021-05-19T01:53:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The xsize_t helper aims to safely convert an off_t to a size_t,\nerroring out when a file offset is too large to fit into a memory\naddress. It does this by using two casts:\n\n\tsize_t size = (size_t) len;\n\tif (len != (off_t) size)\n\t\t... error out ...\n\nOn a platform with sizeof(size_t) < sizeof(off_t), this check is safe\nand correct. The first cast truncates to a size_t by finding the\nremainder modulo SIZE_MAX+1 (see C99 section 6.3.1.3 Signed and\nunsigned integers) and the second promotes to an off_t, meaning the\nresult is true if and only if len is representable as a size_t.\n\nOn other platforms, this two-casts strategy still works well (always\nsucceeds) for len >= 0. But for len < 0, when the first cast succeeds\nand produces SIZE_MAX + 1 + len, the resulting value is too large to\nbe represented as an off_t, so the second cast produces implementation\ndefined behavior. In practice, it is likely to produce a result of\ntrue despite len not being representable as size_t.\n\nSimplify by replacing with a more straightforward check: compare len\nto the relevant bounds and then cast it. (To avoid a -Wsign-compare\nwarning, after checking that len >= 0, we explicitly convert to a\nsufficiently-large unsigned type before comparing to SIZE_MAX.)\n\nIn practice, this is not likely to come up since typical callers use\nnonnegative len. Still, it's helpful to handle this case to make the\nbehavior easy to reason about.\n\nHistorical note: the original bounds-checking in 46be82dfd0 (xsize_t:\ncheck whether we lose bits, 2010-07-28) did not produce this\nimplementation-defined behavior, though it still did not handle\nnegative offsets. It was not until 73560c793a (git-compat-util.h:\nxsize_t() - avoid -Wsign-compare warnings, 2017-09-21) introduced the\ndouble cast that the implementation-defined behavior was triggered.\n\nSigned-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> -\tif (len != (off_t) size)\n>> +\tif (len < 0 || len > SIZE_MAX)\n>> \t\tdie(\"Cannot handle files this big\");\n>\n> OK, so negative offset or offset that cannot be represented as size_t\n> are rejected. That is much easier to read than the original ;-)\n>\n> SIZE_MAX is associated with size_t so it presumably is an unsigned\n> constant; would it again trigger a sign-compare warning?\n\nAlas, on platforms with sizeof(size_t) == sizeof(off_t), I believe it\ndoes:\n\n\t$ gcc --version\n\tgcc (Debian 10.2.1-6+build2) 10.2.1 20210110\n\tCopyright (C) 2020 Free Software Foundation, Inc.\n\tThis is free software; see the source for copying conditions. There is NO\n\twarranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n\n\t$ cat sign-compare-test.c\n\t#define X (1U)\n\n\textern int signed_int();\n\n\tint main(void)\n\t{\n\t int v = signed_int();\n\t return v < 0 || v > X;\n\t}\n\t$ gcc -c -Wall -W -Wsign-compare sign-compare-test.c\n\tsign-compare-test.c: In function ‘main’:\n\tsign-compare-test.c:8:21: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare]\n\t 8 | return v < 0 || v > X;\n\t | ^\n\nThat can be worked around by reintroducing a cast, to an unsigned type\nthis time, like this.\n\nThanks,\nJonathan\n\n git-compat-util.h | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex a508dbe5a3..fb6e9af76b 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -986,11 +986,9 @@ static inline char *xstrdup_or_null(const char *str)\n \n static inline size_t xsize_t(off_t len)\n {\n-\tsize_t size = (size_t) len;\n-\n-\tif (len != (off_t) size)\n+\tif (len < 0 || (uintmax_t) len > SIZE_MAX)\n \t\tdie(\"Cannot handle files this big\");\n-\treturn size;\n+\treturn (size_t) len;\n }\n \n __attribute__((format (printf, 3, 4)))\n-- \n2.31.1.818.g46aad6cb9e\n\n"}]}