buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51531, getsockopt/SO_ERROR not returning...

 
 

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

bug #51531: getsockopt/SO_ERROR not returning value stored in errno

Submitter:  Joel Cunningham <jcunningham>
Submitted:  Fri 21 Jul 2017 09:29:03 PM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Tue 25 Jul 2017 07:54:48 PM UTC, comment #8: 

So let's remove LWIP_SOCKET_SET_ERRNO then. My understanding is that a "proper" OS/C-library combination nowadays should have TLS errno support built in (and all I used did). If it's too lightweight, it's probably not worth using sockets anyway.

-> I've removed it LWIP_SOCKET_SET_ERRNO.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 25 Jul 2017 07:20:45 PM UTC, comment #7: 


> In my opinion, we had to remove the SO_ERROR handling to meet the standards.


I agree, I'm in favor of the standard behavior :)  Before David commented, I didn't have correct understanding of the way SO_ERROR behaved on POSIX.  errno with TLS is working on my platforms, so I can upgrade my old applications to use SO_ERROR correctly.

My suggestion of updating the UPGRADING file and remove LWIP_SOCKET_SET_ERRNO was to make it clear that the previous use case is no longer available.  I don't have an opinion about getting this use case working again since all of my platforms have errno with TLS

Joel Cunningham <jcunningham>
Group Member
Tue 25 Jul 2017 06:42:20 PM UTC, comment #6: 

In my opinion, we had to remove the SO_ERROR handling to meet the standards.

Of course it's true that we now require thread-local storage for 'errno'. So from this point of view, keeping the error per socket is more lightweight.

But then, how lightweight is the socket API at all? Do we need an lwIP-special way of error handling? Maybe we shouldn't remove LWIP_SOCKET_SET_ERRNO but let 'set_errno()' pass the socket, too. Then, everyone having thread-local storage problems could store the error like they want, giving up standards compatibility on their own...?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 24 Jul 2017 08:49:27 PM UTC, comment #5: 


> I've just added this to UPGRADING.


Great thanks!

> Hmm, I never thought of socket application being tailored to lwIP's implementation of the standard. We're trying to implement the standard behaviour as documented at opengroup.org. The behaviour was wrong before (and yes, it was easier fixing an old bug doing it the right way :-)


I had some old application code that was written for a platform where LWIP_SOCKET_SET_ERRNO was set to FALSE (due to platform issues with errno not being thread safe) and thus it used getsockopt with SO_ERROR in all the places you would normally check errno.  As we've determined, the old behavior of SO_ERROR was wrong, but it did enable a use case where applications could use sockets without errno support.

Maybe we need to remove LWIP_SOCKET_SET_ERRNO since you can no longer get those errors via SO_ERROR and we don't want to lead developers to thinking errno is optional (for sockets layer)?

Joel Cunningham <jcunningham>
Group Member
Mon 24 Jul 2017 07:16:43 PM UTC, comment #4: 

I've just added this to UPGRADING.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 24 Jul 2017 06:25:45 PM UTC, comment #3: 


> Our UPGRDING file needs a description of this required
> integration as existing LwIP applications are going to be broken.


Hmm, I never thought of socket application being tailored to lwIP's implementation of the standard. We're trying to implement the standard behaviour as documented at opengroup.org. The behaviour was wrong before (and yes, it was easier fixing an old bug doing it the right way :-)

Of course, documenting this in UPGRADING can't harm!

Simon Goldschmidt <goldsimon>
Group administrator
Mon 24 Jul 2017 03:17:02 PM UTC, comment #2: 

David,

Thanks for clarifying the behavior of SO_ERROR. I read over through Steven's Unix Networking Programmig Vol 1 and agree that from what he describes it only stores asynchronous errors that occur outside of application function calls and not as analog to errno. Interestingly on Solaris he even mentions that SO_ERROR only migrates the pending error to errno and does not return the value in th opt field

Our UPGRDING file needs a description of this required integration as existing LwIP applications are going to be broken. This also means that errno is no longer optional for the sockets API. Might want to audit the options to since errno was once optional and applications could get by with using SO_ERROR only

Joel Cunningham <jcunningham>
Group Member
Fri 21 Jul 2017 11:20:29 PM UTC, comment #1: 

What you're describing is expected behavior. Once an error has made it into 'errno', ht will not be available through SO_ERROR anymore. SO_ERROR is effectively only to be used for cases where there was no possibility to move the error code into 'errno'. As such, your application logic will not work on other POSIX-compliant systems either.

David van Moolenbroek <dcvmoole>
Fri 21 Jul 2017 09:29:03 PM UTC, original submission:  

I've found that getsocktop with SO_ERROR is not returning the value stored in errno.

The particular application case I'm working with is non-blocking connect.  connect() returns -1 and then I call getsockopt and get 0, leading my application to think the connection has completed early.  I then call send() which returns with an error

I did some debugging and can see that the ERR_INPROGRESS is being returned from lwip_netconn_do_connect() and EINPROGRESS is set in errno.  But then getsockopt with SO_ERROR calls netconn_err(), which checks conn->pending_err.  pending_err doesn't appear to be set in the connect path, but only from the TCP err callback

Joel Cunningham <jcunningham>
Group Member

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dcvmoole (Posted a comment)
  • -email is unavailable- added by jcunningham (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-24 jcunningham StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2017-07-24 jcunningham StatusFixed None
        Assigned togoldsimon None
        Open/ClosedClosed Open
    2017-07-24 goldsimon StatusNone Fixed
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code