buglwIP - A Lightweight TCP/IP stack - Bugs: bug #54062, Inconsequent null pointer checks

 
 

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

bug #54062: Inconsequent null pointer checks

Submitter:  Matthias Hofmann <mhofmann>
Submitted:  Tue 05 Jun 2018 09:48:27 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  2.1.0
lwIP version:  2.0.3

Jump to the original submission

Thu 14 Jun 2018 02:21:15 PM UTC, comment #20: 

Pushed, thanks for the patch.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 14 Jun 2018 11:44:18 AM UTC, comment #19: 

OK, sorry. I'll take a look again.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 14 Jun 2018 11:43:09 AM UTC, comment #18: 

It looks like some got lost during changing the patch for master and some were filtered by you.



(file #44367)

Matthias Hofmann <mhofmann>
Wed 13 Jun 2018 12:02:06 PM UTC, comment #17: 

In comment #6 you wrote:
"With the attached patch null pointer usage can be detected reliable"

So have you forgotten functions or did I miss them when filtering your patch?

In any case, feel free to post another patch.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 13 Jun 2018 08:01:18 AM UTC, comment #16: 

There are multiple functions that dereference pointer arguments without checking them:

netif.c
- netif_set_ipaddr
- netif_set_gw
- netif_set_netmask

tcp_out.c
- tcp_output_alloc_header
- tcp_send_fin
- tcp_build_wnd_scale_option
- tcp_send_empty_ack

If for these function NULL is explicitly allowed, they need to be modified. Currently they will crash if NULL is used as argument.

Matthias Hofmann <mhofmann>
Tue 12 Jun 2018 08:26:13 AM UTC, comment #15: 

Pushed, thanks for the patch. I still had to clean up multiple things, so you might want to check if you're OK with the current status.

Things I cleaned up include:
- function name copy/paste bugs in error strings
- mixed code and declarations
- NULL checks where NULL is explicitly allowed

Simon Goldschmidt <goldsimon>
Group administrator
Mon 11 Jun 2018 07:52:21 AM UTC, comment #14: 

There's no ongoing development on the STABLE-2_0_0 branch, so no separate patches are needed. Just one for master.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 11 Jun 2018 07:50:18 AM UTC, comment #13: 

The differences between master and STABLE-2_0_0 are a little bit bigger. So you need different patches for master and stable branch:

- Patch 2 is for STABLE-2_0_0
- Patch 3 is for master

(file #44357)

Matthias Hofmann <mhofmann>
Fri 08 Jun 2018 09:11:11 PM UTC, comment #12: 

Please rebase to git master, applying the patch fails.

Simon Goldschmidt <goldsimon>
Group administrator
Fri 08 Jun 2018 07:45:34 PM UTC, comment #11: 

Ok, LWIP_ERROR and LWIP_ASSERT are flexible enough to be replaced by space-saving version on demand. So, error messages prefixed by function name are better.

Jan Breuer <jan_breuer>
Fri 08 Jun 2018 01:53:27 PM UTC, comment #10: 

The reworked patch with long error messages is done.

(file #44332)

Matthias Hofmann <mhofmann>
Fri 08 Jun 2018 01:06:05 PM UTC, comment #9: 

As a user I know I would definitely prefer see and debug "udp_input: invalid pcb" instead of "invalid pcb".

That extra 11 bytes of error message could easily save me an hour of digging and breakpointing. In my opinion it is well worth it.

-Jason White

Jason White <wrc_jasonw>
Fri 08 Jun 2018 09:06:35 AM UTC, comment #8: 

For the error messages I have oriented towards api_lib.c style.

What is the preferred style for the error messages? The short form like "invalid pcb", or the long form "udp_input: invalid pcb"?

Matthias Hofmann <mhofmann>
Thu 07 Jun 2018 01:40:38 PM UTC, comment #7: 

Some notes to your patch

Adding function name as prefix to all error messages is nice but it makes code unnecessary bigger. It is not a problem for debug messages but it can be a problem for production builds.

LWIP_ASSERT and LWIP_ERROR can't be selectively disabled per functionality like LWIP_DEBUGF can. So, you have them all or you don't have them at all. Be careful by adding lot of new long texts.

There is still ability to implement custom LWIP_ERROR/LWIP_ASSERT which will add macros like _FILE_, _LINE_, _func_ or any other text available by your compiler.



There are some copy/paste bugs like
+  LWIP_ERROR("netif_set_addr, invalid netif", netif != NULL, return);
+  LWIP_ERROR("netif_set_addr, invalid netif", netmask != NULL, return);
+  LWIP_ERROR("netif_set_addr, invalid netif", gw != NULL, return);

There is inconsistency with starting the error message
+   LWIP_ASSERT("tcp_remove_listener: invalid pcb list", list != NULL);
vs
+  LWIP_ASSERT("netif_get_ip6_addr_match, invalid netif", netif != NULL);

Sometimes you use "," and sometimes ":". I think, that preferable is ":", because it is already used in the source code.

Jan Breuer <jan_breuer>
Thu 07 Jun 2018 01:10:24 PM UTC, comment #6: 

With the attached patch null pointer usage can be detected reliable.

(file #44321)

Matthias Hofmann <mhofmann>
Thu 07 Jun 2018 05:29:52 AM UTC, comment #5: 

I'm working on a patch according to your advice. When it is done we can close the bug and additional rework can be provided as patches.

When adding the check code I stumbled over different return codes for NULL checks. For example netconn_getaddr use ERR_ARG, but in the UDP code ERR_VAL is used.

Is there any difference between both return codes and when to use them?

I tend to use ERR_ARG for null checks.

Matthias Hofmann <mhofmann>
Wed 06 Jun 2018 07:26:12 PM UTC, comment #4: 

Well, you can't say it like that. You have to differentiate:
- things that can go wrong from external input must always be handled gracefully to prevent DoS attacks. A DEBUGF output may be printed and an error counter may be increased instead.

For the rest, handling is a bit unclear as
- LWIP_ASSERT can simply be disabled but the stack should continue to work, so this rather covers errors in algorithms
- LWIP_ERROR can be disabled, but it's not meant to. This covers checking invalid input values and can be set to "halt" on error to catch errors during development and be set to just return an error (without DEBUGF) in production code.

However, the code has not been clean up everywhere to follow this rule.

Feel free to post patches. But I'm not sure if this is worth keeping this bug entry open?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 06 Jun 2018 10:00:29 AM UTC, comment #3: 

So LWIP_ERROR shall be used for null pointer checks? As I see it is already used for the higher API layers netconn and socket.

Only the low level, callback style API use no LWIP_ERROR checks. I can take care of it, if nobody other want to do it ;)

Matthias Hofmann <mhofmann>
Wed 06 Jun 2018 08:00:14 AM UTC, comment #2: 

We've had a lot of discussion about LWIP_ASSERT, although that is some years ago. The question is whether you have it enabled at runtime or not and if you still check null pointers when LWIP_ASSERT is disabled or not...

The discussion lead to adding LWIP_ERROR, but the usage never really got cleaned up, I think... :-(

Simon Goldschmidt <goldsimon>
Group administrator
Tue 05 Jun 2018 04:24:59 PM UTC, comment #1: 

As a user I would love to see more LWIP_ASSERT coverage.

Frankly debugging is challenging enough on microcontrollers even without stay pointers...

-Jason White

Jason White <wrc_jasonw>
Tue 05 Jun 2018 09:48:27 AM UTC, original submission:  

Hey,

how are the rules for null pointer checks? Some functions contain null pointer checks, some not.

A bug in our software leads to "funny", hard to debug results as a null pointer traversed over tcp_abort() into the lwip Stack.
Checking the TCP RAW API some function handle null pointer gracefully by returning an error, some check for null pointer via LWIP_ASSERT, some just use pointers.

It would be nice if the lwip API function have a common behaviour handling null pointers.
What are your opinions?

Cheer Matthias

Matthias Hofmann <mhofmann>

 

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

Attached Files
file #44367:  0004-Add-missing-null-checks.patch added by mhofmann (2KiB - application/octet-stream)
file #44357:  0003-Add-null-pointer-checks-for-TCP-UDP-and-netif_for-master.patch added by mhofmann (30KiB - application/octet-stream - Patch for master branch)
file #44332:  0002-Add-null-pointer-checks-for-TCP-UDP-and-netif.patch added by mhofmann (29KiB - application/octet-stream - Reworked Patch - Long error messages)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jan_breuer (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by wrc_jasonw (Posted a comment)
  • -email is unavailable- added by mhofmann (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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-06-14 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-06-14 goldsimon Item GroupNone Change Request
        StatusFixed In Progress
        Open/ClosedClosed Open
        Planned ReleaseNone 2.1.0
    2018-06-14 mhofmann Attached File- Added 0004-Add-missing-null-checks.patch, #44367
    2018-06-12 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2018-06-11 mhofmann Attached File- Added 0003-Add-null-pointer-checks-for-TCP-UDP-and-netif_for-master.patch, #44357
    2018-06-08 mhofmann Attached File- Added 0002-Add-null-pointer-checks-for-TCP-UDP-and-netif.patch, #44332
    2018-06-07 mhofmann Attached File- Added 0001-Add-null-pointer-checks-for-TCP-UDP-and-netif.patch, #44321

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code