{"thread":{"id":"42765","subject":"[PATCH] config.c: fix potential number truncation in git_parse_signed()","startedAt":"2016-07-02T13:13:51Z","lastAt":"2016-07-08T16:02:25Z","messageCount":3,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"290749","messageId":"20160702131333.8418-1-pclouds@gmail.com","threadId":"42765","inReplyTo":null,"subject":"[PATCH] config.c: fix potential number truncation in git_parse_signed()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-02T13:13:33Z","receivedAt":"2016-07-02T13:13:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"clang -Wabsolute-value on IA-32 architecture complains that \"absolute\nvalue function 'labs' given an argument of type 'intmax_t' (aka 'long\nlong') but has parameter of type 'long' which may cause truncation of\nvalue\". Very unlikely for this code though. Nevertheless, add an\nexplicit check for truncation to shut clang up and error out.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n config.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex d7ce34b..880bd4a 100644\n--- a/config.c\n+++ b/config.c\n@@ -503,6 +503,7 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\tintmax_t val;\n \t\tuintmax_t uval;\n \t\tuintmax_t factor = 1;\n+\t\tlong int lival;\n \n \t\terrno = 0;\n \t\tval = strtoimax(value, &end, 0);\n@@ -512,9 +513,14 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\t\terrno = EINVAL;\n \t\t\treturn 0;\n \t\t}\n-\t\tuval = labs(val);\n+\t\tlival = (long int)val;\n+\t\tif (lival != val) {\n+\t\t\terrno = ERANGE;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tuval = labs(lival);\n \t\tuval *= factor;\n-\t\tif (uval > max || labs(val) > uval) {\n+\t\tif (uval > max || labs(lival) > uval) {\n \t\t\terrno = ERANGE;\n \t\t\treturn 0;\n \t\t}\n-- \n2.8.2.532.g6dfa503.dirty\n\n"},{"id":"290907","messageId":"xmqqa8husgrj.fsf@gitster.mtv.corp.google.com","threadId":"42765","inReplyTo":"20160702131333.8418-1-pclouds@gmail.com","subject":"Re: [PATCH] config.c: fix potential number truncation in git_parse_signed()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-06T19:33:52Z","receivedAt":"2016-07-06T19:34:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> clang -Wabsolute-value on IA-32 architecture complains that \"absolute\n> value function 'labs' given an argument of type 'intmax_t' (aka 'long\n> long') but has parameter of type 'long' which may cause truncation of\n> value\". Very unlikely for this code though. Nevertheless, add an\n> explicit check for truncation to shut clang up and error out.\n\nThanks. It however makes me wonder if it is a better approach to\navoid downcasting intmax_t to long by using imaxabs()?\n\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> config.c | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index d7ce34b..880bd4a 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -503,6 +503,7 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n> \t\tintmax_t val;\n> \t\tuintmax_t uval;\n> \t\tuintmax_t factor = 1;\n> +\t\tlong int lival;\n> \n> \t\terrno = 0;\n> \t\tval = strtoimax(value, &end, 0);\n> @@ -512,9 +513,14 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n> \t\t\terrno = EINVAL;\n> \t\t\treturn 0;\n> \t\t}\n> -\t\tuval = labs(val);\n> +\t\tlival = (long int)val;\n> +\t\tif (lival != val) {\n> +\t\t\terrno = ERANGE;\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t\tuval = labs(lival);\n> \t\tuval *= factor;\n> -\t\tif (uval > max || labs(val) > uval) {\n> +\t\tif (uval > max || labs(lival) > uval) {\n> \t\t\terrno = ERANGE;\n> \t\t\treturn 0;\n> \t\t}\n"},{"id":"291033","messageId":"CACsJy8DEC1BqC8q0KDa5ZhCu6zFWemgNWoO-vRT0PgcZ616Svg@mail.gmail.com","threadId":"42765","inReplyTo":"xmqqa8husgrj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] config.c: fix potential number truncation in git_parse_signed()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-07-08T16:01:47Z","receivedAt":"2016-07-08T16:02:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 6, 2016 at 9:33 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> clang -Wabsolute-value on IA-32 architecture complains that \"absolute\n>> value function 'labs' given an argument of type 'intmax_t' (aka 'long\n>> long') but has parameter of type 'long' which may cause truncation of\n>> value\". Very unlikely for this code though. Nevertheless, add an\n>> explicit check for truncation to shut clang up and error out.\n>\n> Thanks. It however makes me wonder if it is a better approach to\n> avoid downcasting intmax_t to long by using imaxabs()?\n\nI think the tricky part is detect overflow. WIth uval being a\npotentially signed number, i'm not sure how to catch it. The (uval >\nmax || labs(val) > uval) was written with unsigned number in mind, I\nthink.\n\n>\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> config.c | 10 ++++++++--\n>> 1 file changed, 8 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/config.c b/config.c\n>> index d7ce34b..880bd4a 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -503,6 +503,7 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>> intmax_t val;\n>> uintmax_t uval;\n>> uintmax_t factor = 1;\n>> + long int lival;\n>>\n>> errno = 0;\n>> val = strtoimax(value, &end, 0);\n>> @@ -512,9 +513,14 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>> errno = EINVAL;\n>> return 0;\n>> }\n>> - uval = labs(val);\n>> + lival = (long int)val;\n>> + if (lival != val) {\n>> + errno = ERANGE;\n>> + return 0;\n>> + }\n>> + uval = labs(lival);\n>> uval *= factor;\n>> - if (uval > max || labs(val) > uval) {\n>> + if (uval > max || labs(lival) > uval) {\n>> errno = ERANGE;\n>> return 0;\n>> }\n\n\n\n-- \nDuy\n"}]}