Numerous misuses of Packet::uniqueify

[pallas]

There are numerous misuses of Packet::uniqueify in many click Elements, specifically related to what happens when uniqueify fails to generate a WritablePacket. The semantic is that uniqueify returns NULL and the original packet loses a reference, that is should be assumed to be gone. In practice, uniqueify will only fail in this way if there are multiple refs already, which means the original Packet probably looks the same for a little while but will disappear mysteriously later. The result are strange bugs under low memory conditions or when allocating while atomic.

In several cases, e.g. fasttcpflows, the result of uniqueify is not checked and there is a comment that says "better not fail." In several cases, the original packet is used, pushed, or killed.

I plan to work on this at some point, but I'm not there yet. I took a quick survey by forcing Packet::uniqueify to always take the expensive path, disabling packet pooling, and causing expensive_uniqueify to fail 10% of the time. With valgrind's track-origins, code with this issue lights right up.

[tbarbette]

I the freeing behavior is actually true in FastClick. A change (quite undocumented but discussed recently in #101), in FastClick is that we have a pool of packet, and a pool of packet with buffer (while the second in vanilla is a pool of buffer only).
As a result the code of uniqueify has a single allocation that do not depend on the number of references. Later, after the packet+buffer allocation, if we actually did not need the new packet because the use count of original was 1, the new packet is freed.

But that being said, even in vanilla I see a return 0 if either the packet alloc is 0 or the buffer alloc is 0 ?

However, there is still the problem of all code paths not checking if return of uniqueify is null.

[pallas]

Yes, it doesn't matter what was backing it, the original packet should be treated as gone unless it was explicitly given another reference in advance. So killing or pushing the original on failure is also a bug. I haven't fully done that audit yet. ~Derrick • iPhone

…

On Oct 31, 2018, at 1:24 AM, Tom Barbette ***@***.***> wrote: I the freeing behavior is actually true in FastClick. A change (quite undocumented but discussed recently in #101), in FastClick is that we have a pool of packet, and a pool of packet with buffer (while the second in vanilla is a pool of buffer only). As a result the code of uniqueify has a single allocation that do not depend on the number of references. Later, after the packet+buffer allocation, if we actually did not need the new packet because the use count of original was 1, the new packet is freed. But that being said, even in vanilla I see a return 0 if either the packet alloc is 0 or the buffer alloc is 0 ? However, there is still the problem of all code paths not checking if return of uniqueify is null. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.