Re: [lwip-users] Bug in pbuf.c regarding PBUF_POOL

[Jonathan Larmour]

Leon Woestenberg wrote:
> > Separately, there's the issue that the !SYS_LIGHTWEIGHT_PROT code is 
> > broken. Someone interested in that code (Christian?) may want to take that up.
>
> I think it the raw (core) API level is not broken. Also, the core is
> intentionally designed to be void of any locking mechanisms.

Go back to the start of this thread: pbuf_pool_free_lock is never changed, 
and pbuf_pool_free_sem is only used by PBUF_POOL_FREE and nowhere else. 
There is therefore no protection between allocs and frees.

Failure scenario with two threads, FREE_T1 doing free, ALLOC_T2 doing alloc:

FREE_T1 calls PBUF_POOL_FAST_FREE
FREE_T1 does p->next = pbuf_pool (where p is pbuf to free)
ALLOC_T2 does p = pbuf_pool
ALLOC_T2 does pbuf_pool = p->next (where p was from previous line)
then later when FREE_T1 is rescheduled:
FREE_T1 does pbuf_pool = p (where p is pbuf to free)

This results in pbuf_pool->next pointing to a buffer that has been 
allocated. If there are two more pbuf allocations, then the second one 
will go horribly wrong, and disaster is inevitable since there will be a 
single pbuf in the pbuf_pool list mentioned twice.

Or if you are saying "void of any locking mechanism", then why even have 
the locks and semaphores that are there for the !SYS_LIGHTWEIGHT_PROT 
code. That's what I had been arguing for removing, after all.

This may overlap with the other mail I sent earlier, but your comment 
seems to agree with the hypothesis that you couldn't have multiple threads 
using the stack simultaneously when using the raw API (in which case there 
needn't be any code to protect it).

> I *do* think the sys (wrapper) level is broken, and has always been
> broken, in corner case of the sys (wrapper) level. I have given examples
> of this years ago, but as no user seemed to hit, or care, about these
> corner cases, the code was mostly kept as-is. I've seen several fixes to
> the sys API code addressing the corner cases.

I'd be interested to know what corner cases remain. Other than the obvious 
finite resources, the abstraction appears crude (which for a lightweight 
stack is not necessarily a bad thing) but functional to me.

> > (depending on implementation), and definitely so in the presence of SLIP or 
> > PPP, and the only present way to solve that is implementing 
> > SYS_LIGHTWEIGHT_PROT. At least in the current code.
>
> This might be true for the sys arch API, as said, I doubt the raw API
> needs any fixing.

You mean sequential rather than sys arch?

> I know it is a nuisance to manage code that has several API's and/or can
> be used in multiple ways. My personal preference would be to throw away
> the sys arch abstraction and rewrite it altogether, merging it with a
> BSD alike socket interface. However, we never made it that far.

I don't know quite what you mean by "alike", but the differences of the 
sequential API from the BSD API are, to me, a very good thing for a 
lightweight stack.

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine