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

Submitted by:  Joel Cunningham <jcunningham>
Submitted on:  Fri 21 Jul 2017 09:29:03 PM UTC  
 
Category: sockets/netconnSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: git head

(Jump to the original submission 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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project AdministratorIn charge of this item.
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>
Project Member
Mon 24 Jul 2017 07:16:43 PM UTC, comment #4:

I've just added this to UPGRADING.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project 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>
Project Member

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by dcvmoole (Posted a comment)
  • -unavailable- added by jcunningham (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 24 Jul 2017 08:51:30 PM UTCjcunninghamStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed
    Mon 24 Jul 2017 08:49:27 PM UTCjcunninghamStatusFixed=>None
      Assigned togoldsimon=>None
      Open/ClosedClosed=>Open
    Mon 24 Jul 2017 07:16:43 PM UTCgoldsimonStatusNone=>Fixed
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1