tasklwIP - A Lightweight TCP/IP stack - Tasks: task #10369, Various changes

 
 

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

task #10369: Various changes

Submitter:  Iordan Neshev <iordan_neshev>
Submitted:  Wed 05 May 2010 08:28:40 PM UTC
   
 
Category:  None Should Start On:  Wed 05 May 2010 12:00:00 AM UTC
Should be Finished on:  Wed 05 May 2010 12:00:00 AM UTC Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Percent Complete:  100%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Wed 12 May 2010 07:48:37 AM UTC, comment #4: 


>> change pbuf->ref from u16_t to s16_t to check for underflows?
> We'd better not. After all, the nature of ref is unsigned.

I think so, too.

>> Add a SIO_FD_X printf-formatter define
> Yet another formatter? Casting to u32_t would do the job


On 64bit architectures, you might get a warning when casting a pointer to 32bit. Instead, I'll leave the cast to size_t and use SZT_F as the printf-formatter. On 32bit platforms, this should be the same.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 12 May 2010 07:25:43 AM UTC, comment #3: 


>change pbuf->ref from u16_t to s16_t to check for underflows?

We'd better not. After all, the nature of ref is unsigned.

>Add a SIO_FD_X printf-formatter define

Yet another formatter? Casting to u32_t would do the job
-----

> 2. ppp_oe.c

oh, you're right. I didn't look at it from this point of view. Sorry if we have already talked about this, I don't remember.

Iordan Neshev <iordan_neshev>
Tue 11 May 2010 06:14:03 PM UTC, comment #2: 

That leaves us with:
- change pbuf->ref from u16_t to s16_t to check for underflows? (can't happen as long as anyone messes around with ->ref without using the pbuf_* functions)

- Add a SIO_FD_X printf-formatter define?

For the rest, I'd wait for changes in pppd.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 11 May 2010 06:12:15 PM UTC, comment #1: 


> 1. In file pbuf.c, line 566 there is ...


I don't think that casting to s32_t changes anything since it would result in an s32_t between 0 and 0x0000ffff, which would never be negative. Casting to s16_t might work, but would limit the current ref-count ot 32K, which shouldn't be a problem, but still, if it was meant like that, I'd rather change the structure definition to s16_t.

> 2. ppp_oe.c
> sys_untimeout should be UNTIMEOUT (lines 286, 618, 631 and 1211)


Hm, ppp_oe.c is a file written only for lwIP and my opinion is that it therefore doesn't have to use the compatibility-defines (UN)TIMEOUT, which were introduced for the pppd code (where TIMEOUT first calls sys_untimeout and then sys_timeout). (After all, if UNTIMEOUT was used, than we would have to use TIMEOUT, too...)

> 3. ppp.c : nPut(PPPControl pc, struct pbuf nb)


Hmm, that one is tricky. The current cast isn't correct since size_t is unsigned (and may be larger than expected for %d) while %d expects (signed-)int. However, we don't have any way of knowing which printf-formatter is correct for sio_fd_t, so for fixing this correctly, we would have to introduce a SIO_FD_X define.

> 4. ppp/fsm.c: function fsm_sconfreq(fsm *f, int retransmit)


I'd agree with you there, but that's not the only place where the present progressive is used instead of the simple past, and I wouldn't just change it in one place...

> 5. ...


Wouldn't we write beyond the buffer (and thus overwrite another memory area) with p[len] = 0 if len is the size of the buffer that p points to???

Aside from that, I wouldn't want to change that unless it's in changed in pppd: I'm not planning lwIP's ppp to become a real fork of the original pppd with lots of changes (after all, if I had the time, I'd completely upgrade to 2.4.5 and use ifdef/macros to disable the stuff we don't need...).

> 6. ...


I've added the call to lcp_close. In lwIP, the state is changed by calling pppIOCtl, so this is already covered.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 05 May 2010 08:28:40 PM UTC, original submission:  

I have several proposals:

1. In file pbuf.c, line 566 there is
    LWIP_ASSERT("pbuf_free: p->ref > 0", p->ref > 0);

Since p->ref is decremented on the next line, if underflow
occurs, the assert would not fire. Casting to s32_t (or maybe s16_t to be more lightweight?) should solve this:
    LWIP_ASSERT("pbuf_free: p->ref > 0", (s32_t)p->ref > 0); /* cast to s32 to detect u16_t underflow */

2. ppp_oe.c
   sys_untimeout should be UNTIMEOUT (lines 286, 618, 631 and 1211)

3. ppp.c : nPut(PPPControl *pc, struct pbuf *nb)

there is:  ("PPP nPut: incomplete sio_write(fd:%d, len:%d, c: 0x%"X8_F") c = %d\n", (size_t)pc->fd, b->len, c, c));

pc->fd should be casted to sio_fd_t, not size_t.

4. ppp/fsm.c: function fsm_sconfreq(fsm *f, int retransmit)

on the bottom there is:
 FSMDEBUG(LOG_INFO, ("%s: sending Configure-Request, id %d\n",

I  believe, we should rename "sending" to "sent", because it is quite misleading when you have to read the PPP log, although pppd says "sending". I'll send this to the pppd mailing list too.

5. ppp/fsm.c:  fsm_rtermreq(fsm *f, int id, u_char *p, int len)

There is
    case LS_OPENED:
      if (len > 0) {
        FSMDEBUG(LOG_INFO, ("%s terminated by peer (%p)\n", PROTO_NAME(f), p));
      } else {
        FSMDEBUG(LOG_INFO, ("%s terminated by peer\n", PROTO_NAME(f)));
      }


I propose to change it like this:
      if ((len > 0) && (p != NULL)) {
         p[len] = '\0';         /* Terminate remote message */
         FSMDEBUG(LOG_INFO, ("%s terminated by peer (%s)\n", PROTO_NAME(f), p));
      } else {
        FSMDEBUG(LOG_INFO, ("%s terminated by peer\n", PROTO_NAME(f)));
      }


I've seen it with my eyes that the remote message should be terminated locally, otherwise random garbage appears after it. Also, p may be renamed to msg, because we use p for pbuf, not for char*.

6. ppp/auth.c: auth_withpeer_fail(int unit, u16_t protocol)
In the latest pppd this function has these 2 rows added at the bottom:
    status = EXIT_PEER_AUTH_FAILED;
    lcp_close(unit, "Authentication failed");

We do not use currently status in PPP code, but I think
lcp_close(unit, "Authentication failed");
should be added at the end.

Sorry for not preparing diff files, but my lwip files are pretty different from the CVS ones.

Iordan Neshev <iordan_neshev>

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2010-05-12 goldsimon StatusNone Done
        Percent Complete0% 100%
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code