Thu 07 Dec 2017 11:03:12 AM UTC, comment #11:
Didn't see that "The c argument is an int, the value of which the application shall ensure is a character representable as an unsigned char or equal to the value of the macro EOF" part - these functions are fundamentally broken and should be fixed.
Applied Axel's patch which ensures valid input range, respecting Mike's comments.
Thanks for insisting, Mike!
|
Thu 07 Dec 2017 10:57:46 AM UTC, comment #10:
One more thing: enclosing macro arguments in parentheses would be a good idea, I think.
|
Thu 07 Dec 2017 10:49:29 AM UTC, comment #9:
Looks good.
But eventually fixing all the callers would be a nicer fix, I think.
|
Thu 07 Dec 2017 10:45:35 AM UTC, comment #8:
I see your point now.
Attached the patch for review.
Cast the character to unsigned char to avoid undefined behavior.
Thanks,
Axel
(file #42590)
|
Thu 07 Dec 2017 10:25:40 AM UTC, comment #7:
Looking at the code, I can see that right now the use of ctype macros in ip4_addr.c is broken. It can lead to undefined behaviour. It became broken when local implementation of those macros was replaced with reference to standard macros, which behave differently.
The compiler warnings are genuine. Silencing them just hides the bugs. The bugs need to be fixed either by adjusting lwip_is<...> macros to match original behaviour or by 'fixing the caller' to match the behaviour of standard ctype macros.
By the way, undefined behaviour does not manifest itself every time, so one test does not prove anything.
|
Thu 07 Dec 2017 10:20:45 AM UTC, comment #6:
To make it clear, the previous cast to int is for !LWIP_NO_CTYPE_H case.
so lwip_isdigit/isxdigit/islower/isspace just calls isdigit/isxdigit/islower/isspace.
Since these functions takes int as argument, the cast does not change any behavior because
the compiler will do that anyway.
It's to silence build warning only. (Some build environment consider warnings as error)
And I just test for !LWIP_NO_CTYPE_H case:
char c = -2;
printf("test=%d %d %d %d\n", isdigit(c), isxdigit(c), islower(c), isspace(c));
And I got:
test=0 0 0 0
The result looks fine.
|
Thu 07 Dec 2017 09:46:18 AM UTC, comment #5:
I don't understand this. If it's wrong in the caller, why the cast to int in the first place? To fix compiler warnings? Shouldn't they be fixed in the caller?
Besides, where does it say that lwip_is<whatever> macros must behave just like standard ctype macros? If you are willing to fix compiler warnings this way, you might as well do it in a way that insures that nobody has to 'fix the caller'.
|
Thu 07 Dec 2017 09:38:21 AM UTC, comment #4:
In this case, cast to unsigned int does not help. It's wrong anyway in the caller and needs fix the caller.
|
Thu 07 Dec 2017 09:31:43 AM UTC, comment #3:
The reference you are referring to also says "The c argument is an int, the value of which the application shall ensure is a character representable as an unsigned char or equal to the value of the macro EOF".
I can easily see how an argument to those macros could have a value of -2, for example, which is a no-no.
|
Thu 07 Dec 2017 09:29:19 AM UTC, comment #2:
Can it be that in his setup, the implementation of the given functions are by macros that are potentially broken?
|
Thu 07 Dec 2017 09:26:34 AM UTC, comment #1:
The function signature says they take an int as argument, so the cast is correct. See:
http://pubs.opengroup.org/onlinepubs/009695399/functions/isdigit.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/isxdigit.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/islower.html
http://pubs.opengroup.org/onlinepubs/009695399/functions/isspace.html
|
Thu 07 Dec 2017 09:22:27 AM UTC, original submission:
A recent commit casts arguments to int before invoking ctype macros:
http://git.savannah.nongnu.org/cgit/lwip.git/commit/?id=eb30dbfdc5d458d4b996987d8be9c818743e59e5
If I remember correctly, gcc complains about this since arguments could be negative and this would lead to accessing an array with a negative index.
So I suggest casting arguments to unsigned int instead.
|