ng_pktbuf: fix alignment overwrite issue #3316
Conversation
miri64
added
Type: bug
|
Can you please document these inline functions? It's hard to understand for me, what's their purpose. |
|
+1 for @OlegHahm 's proposal :-) |
| @@ -165,7 +168,7 @@ void *_pktbuf_internal_alloc(size_t size) | |||
|
|
|||
| while ((node->next != NULL) | |||
| /* and if space between current and next allocation is not big enough */ | |||
| && ((_start_idx(node->next) - _end_idx(node)) < _total_sz(size))) { | |||
| && ((_start_idx(node->next) - _end_idx(node)) < _al_sz(_total_sz(size)))) { | |||
There was a problem hiding this comment.
@authmillenon I don't get why it is start - end (simplified). Isn't endalways greater than start? And in that case you'll get negative values there? Just asking dump to get a foolproof answer :-)
There was a problem hiding this comment.
Look again: start - end simplifies too much. It's start(next_node) - end(this_node) so it determines the space between two allocated spaces of 2 nodes. If this is negative, the chunks would overlap, which would be am error ;)
There was a problem hiding this comment.
Obviously I overlooked node->next and node in my error-searching-mania. Sorry! Second stupid question: (Why) is the node->next != NULL condition needed? Besides looking at the code I just wonder if there is a situation where you need to create a new node, because the free memory is too small. How/where do you handle this?
There was a problem hiding this comment.
node->next == NULL means, the current chunk is the last in the packet buffer, so as long as the packet buffer is big enough, exiting the loop here means the new chunk will be appended to the list of nodes. A new node/chunk (a node just describes a chunk that comes immediately after it so the terms are somewhat interchangable) is always created when this function is called (this is the purpose of this function), except of cause if there is no space left (which is checked in https://github.com/RIOT-OS/RIOT/pull/3316/files#diff-2901d789a30fdc0411aca7f3e3ec7249R178).
There was a problem hiding this comment.
Thanks for the detailed feedback! Last one:
A new node/chunk (a node just describes a chunk that comes immediately after it so the terms are somewhat interchangable) is always created when this function is called
Which function exactly or in which line?
There was a problem hiding this comment.
This function we are talking in: _pktbuf_internal_alloc(). The node is "created" in https://github.com/RIOT-OS/RIOT/pull/3316/files#diff-2901d789a30fdc0411aca7f3e3ec7249R176, but only filled after it is determined, that there is enough space available. (https://github.com/RIOT-OS/RIOT/pull/3316/files#diff-2901d789a30fdc0411aca7f3e3ec7249R183 and following)
There was a problem hiding this comment.
@authmillenon, maybe you add some more comments that would help someone like @PeterKietzmann to understand the code better?
There was a problem hiding this comment.
Yeah… or (as I proposed last week) rewrite it as a far less complicated one.
|
Are the added comments helpful? |
| @@ -88,12 +88,10 @@ static inline size_t __total_sz(_used_t *node) | |||
| } | |||
|
|
|||
| /** | |||
| * @brief aligned size with metadata | |||
| * @brief aligns @p size to the next word alignment. | |||
There was a problem hiding this comment.
I'm thinking about an other term for "word". But still idealess...
|
Let's merge this PR as soon as we clarified what's wrong with the unittest. |
What's wrong with the unittest? |
|
The address sanitizer tool points to an issue which is caused by the newly introduced unittest function. Anyway, as you also agree we can merge this PR and fix the issue in an other PR once we found the problem (That's what @authmillenon stated online. I won't NACK that action) |
|
@authmillenon I think you can squash already. |
Currently it can happen if there is a spot of size `n` free that a chunk of size `n + 1` is inserted, if `n` is devisable by the word length of the platform. This patch fixes this issue.
miri64
force-pushed
the
ng_pktbuf/fix/align
branch
from
7d65aaa
to
3b13381
Compare
|
squashed |
|
I encountered a address sanitizer message in the packet buffer unittests when testing #3327, too. Since that one did not show up in @PeterKietzmann's check I'm assuming, though it does show in thes PR in the newly introduced test, it is not related to it. |
|
In combination with #3311 I don't get any fatal errors (segfaults, hanging nodes and the like) with native and ICMPv6 pings. I tried with 300.0000 pings and different payloads (up to 1kB). |
|
Try more, it will break but unrelated to this PR. |
PeterKietzmann
added
the
CI: ready for build
|
So we'll try to figure out the problems and solve them in an other PR. ACK for this one |
ng_pktbuf: fix alignment overwrite issue
Currently it can happen if there is a spot of size
nfree that a chunk of sizen + 1is inserted, ifnis devisable by the word length of the platform. This patch fixes this issue and adds a test to prevent this kind of bug in future implementations.