[patch #5958] netconn_listen & netconn_accept error handling

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

URL:
  <http://savannah.nongnu.org/patch/?5958>

                 Summary: netconn_listen & netconn_accept error handling
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: fbernon
            Submitted on: mercredi 23.05.2007 à 11:21
                Category: None
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: fbernon
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

if your call netconn_accept without any previous netconn_listen call, the
acceptmbox is not valide, and you can have an error (consequence is "port
dependant"). I propose to add a simple guard inside netconn_accept:

if (conn->acceptmbox == SYS_MBOX_NULL) {
  return ERR_VAL;
}

For "listen", the code to allocate the acceptmbox is done inside
netconn_listen AND inside do_listen. I propose to remove the code inside
netconn_listen, because there is a better error handling in do_listen
(netconn type check, check tcp_listen return values...)





    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #1, patch #5958 (project lwip):

agree

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #2, patch #5958 (project lwip):

in api_lib.c, in netconn_accept, it will be :

  if (conn->acceptmbox == SYS_MBOX_NULL) {
    conn->err = ERR_VAL;
    return NULL;
  }

and in sockets.c, in lwip_accept, it will be :

  newconn = netconn_accept(sock->conn);
  if (!newconn) {
    LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_accept(%d) failed, err=%d\n", s,
sock->conn->err));
    sock_set_errno(sock, err_to_errno(sock->conn->err));
    return -1;
  }


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #3, patch #5958 (project lwip):

Since this indicates a coding error, rather than something that can sometimes
happen in real use, should this be an assert?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #4, patch #5958 (project lwip):

That's also true... an ASSERT should be enough.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #5, patch #5958 (project lwip):

I agree with you, but if you want to be coherent, we also should replace in
api_lib function any code like ...

if (conn == NULL) {
  return ERR_VAL;
}

... by an assert, right?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #6, patch #5958 (project lwip):

Yep, assertions sounds good to me.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #7, patch #5958 (project lwip):

Re comment #5, I think yes. In fact these would want Simon's new
LWIP_DEBUG_ASSERT.



    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #8, patch #5958 (project lwip):

Since everyone seems ok, I will replace them by LWIP_DEBUG_ASSERT.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #9, patch #5958 (project lwip):

I think we have somehow agreed in that we want to be able to catch all errors
that don't come from a programmer's fault using normal code flow (return
values) and remove the LWIP_DEBUG_ASSERT...
So you might better use the normal LWIP_ASSERT...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #10, patch #5958 (project lwip):

I don't understand. I thought LWIP_DEBUG_ASSERT was precisely for this sort
of programmer error?

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #11, patch #5958 (project lwip):

And I thought we wanted to get rid of it again and have ASSERT for error
catching and program flow to catch error which can happen in a well-tested
realease mode application. That would imply that ASSERTs only fire when
debugging -> through programmer errors. And then, we would not need the
DEBUG_ASSERT. That was what I thought we discussed in tark #6792... Only we
didn't finish that discussion with a final statement...

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Nachricht geschickt von/durch Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Follow-up Comment #12, patch #5958 (project lwip):

Oh I hadn't looked at Kieran's comments there. I'll reply about that in that
task. LWIP_DEBUG_ASSERT reflects what's in CVS at least.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel
Reply | Threaded
Open this post in threaded view
|

[patch #5958] netconn_listen & netconn_accept error handling

Ondrej Lufinka

Update of patch #5958 (project lwip):

                  Status:                    None => Done                  
             Open/Closed:                    Open => Closed                

    _______________________________________________________

Follow-up Comment #13:

Ok, I check in like initially propose, with LWIP_ASSERT (I suppose than
another task will be open to replace in the whole code the existing
LWIP_ASSERT by the "good one").

Note that netconn_delete doesn't rise an ASSERT if conn==NULL, because on a
err_tcp call, we could post/fetch a NULL netconn in acceptmbox...


    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?5958>

_______________________________________________
  Message posté via/par Savannah
  http://savannah.nongnu.org/



_______________________________________________
lwip-devel mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-devel