buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52611, Incorrect use of ctype macros?

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #52611: Incorrect use of ctype macros?

Submitter:  Mike Kleshov <kleshov>
Submitted:  Thu 07 Dec 2017 09:22:27 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

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!

Dirk Ziegelmeier <dziegel>
Group administrator
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.

Mike Kleshov <kleshov>
Group Member
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.

Mike Kleshov <kleshov>
Group Member
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)

Axel Lin <axellin>
Group Member
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.

Mike Kleshov <kleshov>
Group Member
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.

Axel Lin <axellin>
Group Member
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'.

Mike Kleshov <kleshov>
Group Member
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.

Axel Lin <axellin>
Group Member
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.

Mike Kleshov <kleshov>
Group Member
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?

Stian Sebastian Skjelstad <mywave>
Thu 07 Dec 2017 09:26:34 AM UTC, comment #1: 
Dirk Ziegelmeier <dziegel>
Group administrator
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.

Mike Kleshov <kleshov>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by axellin (Posted a comment)
  • -email is unavailable- added by mywave (Posted a comment)
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by kleshov (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-07 dziegel StatusWont Fix Fixed
        Open/ClosedOpen Closed
    2017-12-07 axellin Attached File- Added 0001-Cast-character-to-unsigned-char-for-isdigit-isxdigit.patch, #42590
    2017-12-07 kleshov Open/ClosedClosed Open
    2017-12-07 dziegel StatusNone Wont Fix
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code