buglwIP - A Lightweight TCP/IP stack - Bugs: bug #2595, loopif results in NULL reference...

 
 

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

bug #2595: loopif results in NULL reference for incoming TCP packets

Submitter:  Andreas Stenius <kaos>
Submitted:  Wed 19 Feb 2003 03:26:30 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Sun 29 Apr 2007 10:35:04 AM UTC, comment #28: 

OK, closing this as fixed as it seems to work ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2007 08:46:00 AM UTC, comment #27: 


> Thank you for the patch. This version works with my
> RAW API FreeRTOS and unixsim build.


That's nice

> BTW, it looks to me that '#include "lwip/mem.h"' could be
> rearranged to #if !LWIP_LOOPIF_MULTITHREADING block as
> mem_malloc() is only used in !MULTITHREADING case.


Yes, and some other includes were missing to be correct. Checked that in.

What's still missing is that instead of PBUF_RAM (which, for now, is in one chunk), other (chained) pbufs could be used. In order to save code I'm planning to modify pbuf_copy() (used for arp queueing) so that it doesn't allocate the target pbuf and can cope with source and target being chained.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2007 08:19:48 AM UTC, comment #26: 

Thank you for the patch.  This version works with my RAW API FreeRTOS and unixsim build.

BTW, it looks to me that '#include "lwip/mem.h"' could be rearranged to #if !LWIP_LOOPIF_MULTITHREADING block as mem_malloc() is only used in !MULTITHREADING case.

Tai-hwa Liang <atliang>
Mon 23 Apr 2007 07:57:17 PM UTC, comment #25: 

I've checked this in, please test!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 Apr 2007 06:31:43 AM UTC, comment #24: 

Thanks for the feedback. I'll check it in one of these days when I find the time creating a comment and adding the define into opt.h.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 23 Apr 2007 01:13:48 AM UTC, comment #23: 

comment #22, supplemental:
3. "LWIP_UNUSED_ARG(ipaddr);" in loopif_output().

Tai-hwa Liang <atliang>
Sun 22 Apr 2007 09:40:40 AM UTC, comment #22: 

Reply to comment #20.  The patch looks okay to me. Here are a few compilation issues I observed whilst building with Paradigm C++ Pro:
1. Need to include <lwip/sys.h> to have SYS_ARCH_DECL_PROTECT()/SYS_ARCH_{UN}PROTECT() support;
2. There's a SYS_ARCH_DECL_PROTECT() in loopif_init() but no correlated SYS_ARCH_{UN}PROTECT().

Tai-hwa Liang <atliang>
Tue 17 Apr 2007 03:06:07 PM UTC, comment #21: 

Ok, I've tested the patch a little and although my application (for which I need the loopif for) does not work, the loopif itself works fine ;-)

Should I check this in or does anyone dislike the extra-configuration option in loopif.h (LOOPIF_MULTITHREADING)?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 17 Apr 2007 08:44:50 AM UTC, comment #20: 

I have created a patch which should solve this problem. Note that to switch between multithreading (call netif->input directly as it puts the packet into an mbox) and raw-API only (put the packet into a linked list, let loopif_poll() fetch it), you have to define LOOPIF_MULTITHREADING to 1 or 0 (since we don't yet have an lwIP-wide switch to know this).

* Note that it does compile, but I did not (yet) have the time to test it.

Any comments are welcome, as I really would like to have a working loopif ;-)

(file #12512)

Simon Goldschmidt <goldsimon>
Group administrator
Tue 17 Apr 2007 08:37:09 AM UTC, comment #19: 

RE comment #18:

The two patches (file #12510/12511) will work, but creating an own thread is not really necessary. It is easier to create a simple linked list of pbufs and call something like loopif_poll() in the main lwip loop (loopif_poll would call netif->input() for every packet in the linked list.

That way, you save yourself from having an additional thread (which saves context-switches and thus, time) and a linked list will be way faster than sys_mbox_t. Also, it's compatible both with NO_SYS=1 and NO_SYS=0.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 17 Apr 2007 02:59:40 AM UTC, comment #18: 

Sorry for the interruption.  I'm the original poster of the thread "local connection failed on loopif, race?" as mentioned in comment #7.  I have done a couple of tests with loopif plus RAW API based client/server on two different platforms(FreeRTOS and unixsim running on FreeBSD).

It's interesting to note that with the current sys_timeout() based implementation, the client hung after sending the first string after connected to the server(for debugging traces, see http://lists.gnu.org/archive/html/lwip-users/2007-03/msg00131.html ).

The attached patches(loopif-q[2].patch) are the thread/mbox based implementation(with slightly modifications) which passed the test described in aforementioned environment. Since slipif, pppif or even ethernetif in many cases would have its own thread to process I/O requests, I think it's reasonable for loopif to put the packet I/O to a different thread context.  The downside is that the patch didn't try to address NO_SYS, yet.

In addition to that, it looks to me that the loopback can be something other than 127/8 as there doesn't appear to be any restriction on this inside the stack.

(file #12510, file #12511)

Tai-hwa Liang <atliang>
Thu 12 Apr 2007 04:22:44 PM UTC, comment #17: 


>Strictly the loopback interface could have other loopback addresses on it, as long as they are within the 127/8 address block.


OK, so no static configuration.

I'll work on a patch and take NO_SYS for the decision to input or not, for the moment. We'll have to decide whether this is OK or not.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 12 Apr 2007 11:43:38 AM UTC, comment #16: 

Strictly the loopback interface could have other loopback addresses on it, as long as they are within the 127/8 address block. I've seen this done on other systems althugh the applications tend to be obscure. So it is possible, people sometimes use it, but it's not normal by any means.

Jonathan Larmour <jifl>
Group Member
Thu 12 Apr 2007 10:59:56 AM UTC, comment #15: 

Yes, you're right about netif and its input function pointer. Note there is always will have to be remove I think.

#if 0 /** TODO: I think this should be enabled, or not? Leon */

Contrib modules initialize loopif like this :

#if LWIP_HAVE_LOOPIF 
  IP4_ADDR(&gw, 127,0,0,1);
  IP4_ADDR(&ipaddr, 127,0,0,1);
  IP4_ADDR(&netmask, 255,0,0,0);
 
  netif_set_default(netif_add(&loopif, &ipaddr, &netmask, &gw, NULL, loopif_init, tcpip_input));
#endif

if it possible/normal to use a loopif with other IP values than these ones? If not, it would be good to declare const in loopif.h/.c.

loopif_poll() is a good idea and a good name.

I also think we have to clarify some define like NO_SYS, SYS_LIGHTWEIGHT_PROT... and to have a explicit define for declare we use tcpip.h/.c (LWIP_MULTITHREAD?), and another one to disable/enable tcpip_callback and associated code is not necessary.

LWIP_MULTITHREAD (or LWIP_SEQUENTIAL_API, or other) will also help to not build tcpip.c, api_msg, api_lib, sockets, if not define (I suppose we can add a #error if these files are build with LWIP_MULTITHREAD=0).

If we change names for NO_SYS & SYS_LIGHTWEIGHT_PROT, we also can add #error to inform and force users about changes...





Frédéric Bernon <fbernon>
Group Member
Thu 12 Apr 2007 06:33:40 AM UTC, comment #14: 

Funny ;-)

Whatever, I think directly calling tcpip_callback() or somethink is not a nice solution. Loopif is a 'normal' netif and has its input function pointer. The only question is when to call it (better: whether to call it or leave that to a polling function).

Maybe we should make it an option for loopif only since we don't seem to know any option until now which tells us in which way the stack is run.

This could also directly includ a function like loopif_poll() (though maybe with a different name?) which has to be called in the main loop when not using the tcpip_thread.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 12 Apr 2007 01:36:31 AM UTC, comment #13: 

Sorry, I'll phrase it differently as I see now it isn't easy English for non-native English speakers:

In the situation I described, some developers will find using the sys API convenient. We can consider forcing them to have to use their own OS native functions instead. I think that would be a good idea, so NO_SYS=0 must mean there is a tcpip_thread. But if so, we need to document this.

Jonathan Larmour <jifl>
Group Member
Thu 12 Apr 2007 01:12:05 AM UTC, comment #12: 


>The question is whether we want to force them to have to do so, even if they might find the sys API convenient. The answer can be yes, but doesn't have to be. Personally I think yes, and if so, we need to document it.


I'm not sure to understand you, my english isn't good enough :) : if you want to say that "users don't have to use lwIP internal features like sys functions or mem functions", yes, I'm agree with you. More, if we can't change some internal features because some developers use them directly, it will be difficult to modify or improve lwIP...

Frédéric Bernon <fbernon>
Group Member
Thu 12 Apr 2007 12:44:34 AM UTC, comment #11: 

I think it is possible for someone to use NO_SYS=0 without tcpip_thread. Just because someone is using the raw API doesn't mean it is a single-threaded system - it may need to interact with other threads.

However, clearly the user could implement that with their own OS native functions rather than the lwip sys API. The question is whether we want to force them to have to do so, even if they might find the sys API convenient. The answer can be yes, but doesn't have to be. Personally I think yes, and if so, we need to document it.


Jonathan Larmour <jifl>
Group Member
Thu 12 Apr 2007 12:07:25 AM UTC, comment #10: 

About comment #8, tcpip_callback doesn't seems to be used, except for PPP, so, associated code would have to be disable, like others tcpip_ functions, to reduce footprint, and only enable if necessary.

Next, if we suppose - and I don't think it's true for all lwIP's users - that loopif can only be use with tcpip_thread, so, perhaps the good thing to do is just to call tcpip_input or tcpip_ethinput (with a pbuf recopy), like Simon suggest in comment #4.

About this last comment, is someone use NO_SYS=0 without tcpip_thread?

Frédéric Bernon <fbernon>
Group Member
Wed 11 Apr 2007 11:01:17 PM UTC, comment #9: 


>prehaps redesign with a mbox + a new thread (but, I don't think it's a good idea)?


In fact, Simon idea in multithread environment is better...

>But still, it is not explicitly documented (I think), that NO_SYS=1 means raw-API/polling only and NO_SYS=0 means netconn/socket-API & tcpip_thread().


But loopif.c can used sys.c functions (sys_timeout) in a raw-api interface, right? So, meaning of NO_SYS you propose seems not right?

Frédéric Bernon <fbernon>
Group Member
Wed 11 Apr 2007 10:52:48 PM UTC, comment #8: 

I think making a call to tcpip_callback() with a suitable callback function and argument would solve a lot of these problems in one fell swoop!

Jonathan Larmour <jifl>
Group Member
Wed 11 Apr 2007 10:43:54 PM UTC, comment #7: 

Just, I also note some remarks in "local connection failed on loopif, race?" (forum's thread March 30th 2007):

If you got a mem_malloc( sizeof( void *[2]))c error, the "r" pbuf is not freed (and "one day" you will got a "not enough memory") Inside sys_timeout, if you got a memp_malloc(MEMP_SYS_TIMEOUT) error, there is no way to know by the caller is the timer is initialized (and to free any resources in this case, and so "one day", you will got a "not enough memory").
Last, using a timer to communicate pbuf between two "contexts" (and in fact, this is the same, tcpip_thread) seems a little strange...

So, things to do seems to be :

- add pbuf_free when "if( NULL == arg ) {" is true
- add a return type de sys_timeout, and check result to do a pbuf_free and a mem_free.
- prehaps redesign with a mbox + a new thread (but, I don't think it's a good idea)?

Frédéric Bernon <fbernon>
Group Member
Wed 11 Apr 2007 06:27:59 PM UTC, comment #6: 


>You could test for ETHARP_TCPIP_INPUT - that's what controls the presence of tcpip_input.


Unfortunately, I think I can't: first it could be either ETHARP_TCPIP_INPUT or ETHARP_TCPIP_ETHINPUT, second, this does not make sure the user is really using the tcpip_thread and not using the polling mode and simply not deactivating the above defines.

I think NO_SYS could be more like it (because it does not make sense to use tcpip_thread() if sys_mbox_fetch() is a null-define...
But still, it is not explicitly documented (I think), that NO_SYS=1 means raw-API/polling only and NO_SYS=0 means netconn/socket-API & tcpip_thread().

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Apr 2007 03:05:45 PM UTC, comment #5: 

You could test for ETHARP_TCPIP_INPUT - that's what controls the presence of tcpip_input.

Jonathan Larmour <jifl>
Group Member
Wed 11 Apr 2007 09:36:13 AM UTC, comment #4: 

This is still open (for a while now...) and I must say I don't like the solution as it introduces 1 ms delay for each packet going over the loopif.

I think there are 2 possibilities solving this:

1. If multithreaded stack is used, netif->input() should be tcpip_input() for the loopif. With that, we can call netif->input directly in loopif_output() since the pbuf is put in a queue.

2. If callback environment is used, we need a simple linked list for incoming packets where we put the pbuf. That queue must then be processed in the main loop, through something like 'poll_driver(netif)' (see http://www.sics.se/~adam/lwip/os.html).

Some other remarks:
- memcpy() into a new pbuf is of course still needed
- using PBUF_POOL would make more sense since this is the type which should be used for input
- the current struct that is malloc'ed using mem_malloc() and passes the netif pointer to loopif_input() is not needed since there can be only one interface with 127.0.0.1 -> the loopif netif struct can be static!

The only thing we would have to know is if we are running in multithreaded or polling mode.

-> Are there any other 'modes' this wouldn't work with?
Hoping for your comments...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 02 Dec 2003 08:15:10 PM UTC, comment #3: 

My application is unfortunately not small, and uses a commercially licensed OS, so I can't provide any usefull source code. But I'll be able to help reproduce the bug on another platform (linux/unix?).

Granted, the patch isn't beatiful, and a more thorough patch to the core problem is welcome.

The most basic app would be to simply add the loopif as default gw, connect and send some data. That is what I did, when it segfaulted (although, I don't get segfaults, it did crash none the less :)

Feel free to mail me for more detailed info..

Andreas Stenius <kaos>
Tue 02 Dec 2003 11:04:35 AM UTC, comment #2: 

is there a simple test program which segfaulted before the patch was applied and works now?
I am curios what this patch works around and see if we could fix the root of the problem.
Andreas?

Jani Monoses <jani>
Group Member
Wed 30 Jul 2003 12:10:15 PM UTC, comment #1: 

Small fix in loopif to let tcp_output finnish off, before sent to ip_input.

Andreas Stenius <kaos>
Wed 19 Feb 2003 03:26:30 PM UTC, original submission:  

When connecting to "localhost" with the loopif interface, tcp_output doesn't get the opportunity to finnish sending the segment before tcp_process gets it, resulting in tcp_process referencing pcb->unacked-> which still is NULL.

Andreas Stenius <kaos>

 

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

Attached Files
file #12512:  loopif_linkedlist.patch added by goldsimon (5KiB - application/octet-stream)
file #12510:  loopif-q.patch added by atliang (4KiB - application/octet-stream)
file #12511:  loopif-q2.patch added by atliang (4KiB - application/octet-stream)
file #567:  loopif.diff added by kaos (1KiB - text/plain - diff file for src/netif/loopif.c)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by atliang (Updated the item)
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2007-04-29 goldsimon StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2007-04-23 goldsimon StatusIn Progress Ready For Test
    2007-04-17 goldsimon Attached File- Added loopif_linkedlist.patch, #12512
    2007-04-17 atliang Attached File- Added loopif-q.patch, #12510
        Attached File- Added loopif-q2.patch, #12511
    2007-04-12 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2003-05-07 likewise Summaryloopif results in NULL reference for incomming TCP packets

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code