patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9288, Possible null-pointer dereference

 
 

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

patch #9288: Possible null-pointer dereference

Submitter:  Mikhail Lappo <mlappo>
Submitted:  Wed 22 Mar 2017 02:45:16 PM UTC
   
 
Category:  sockets/netconn Priority:  7 - High
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Wed 22 Mar 2017 09:36:03 PM UTC, comment #4: 

Mikhail, you decide if you want asserts to trap on your code or not. That assertion is a macro, check if your port turns that into assert or removes it, and check whether your compiler optimizes something out for you.

If you put redundant checks I don't want, then I can't decide ;^)

Anyway, impact is minimal and some people like them. Just pointing out, I don't like them. I rather like assertions on debugging to trap on my coding mistakes.

Sergio R. Caprile <scaprile>
Wed 22 Mar 2017 08:03:29 PM UTC, comment #3: 

Sergio, Is it possible that assert is gone in release build and then redundant check is important?

Mikhail Lappo <mlappo>
Wed 22 Mar 2017 06:06:54 PM UTC, comment #2: 

The possible null pointer was already caught by the assertion:

   LWIP_ASSERT("recv_udp must have an argument", arg != NULL);
   conn = (struct netconn *)arg;
+
+  if (conn == NULL) {
+    pbuf_free(p);
+    return;
+  }
+

conn is arg, and arg is checked for not NULL in the assertion;
so this added piece of code is a redundant check.

Then, the patch correctly removes unnecessary checks within the if's:

-  if ((conn == NULL) || !sys_mbox_valid(&conn->recvmbox) ||
+  if (!sys_mbox_valid(&conn->recvmbox) ||
       ((recv_avail + (int)(p->tot_len)) > conn->recv_bufsize)) {
 #else  /* LWIP_SO_RCVBUF */
-  if ((conn == NULL) || !sys_mbox_valid(&conn->recvmbox)) {
+  if (!sys_mbox_valid(&conn->recvmbox)) {


Sergio R. Caprile <scaprile>
Wed 22 Mar 2017 02:51:54 PM UTC, comment #1: 

Once again, thanks for reporting AND fixing!

Dirk Ziegelmeier <dziegel>
Group administrator
Wed 22 Mar 2017 02:45:16 PM UTC, original submission:  

In assertion the pointer that is potentialy null is dereferenced. The check for null was located after.

Mikhail Lappo <mlappo>

 

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

Attached Files
file #40063:  0001-Possible-null-pointer-dereference.patch added by mlappo (1KiB - text/x-patch - Patch-fix)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-03-22 dziegel StatusNone Done
        Open/ClosedOpen Closed
    2017-03-22 mlappo Attached File- Added 0001-Possible-null-pointer-dereference.patch, #40063
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-aa77.
    Corresponding source code