fix #1147 : init frag variable before use. #1162
Conversation
There was a problem hiding this comment.
Looks good for me, but the fix is very subtle, so a review from @aleksostapenko is very wished.
|
|
||
| BUG_ON(TFW_STR_DUP(data)); | ||
| if (WARN_ON_ONCE(it->frag >= skb_shinfo(it->skb)->nr_frags)) | ||
| return -E2BIG; |
There was a problem hiding this comment.
In case of *skb == skb_head in ss_skb_frag_next() we get here skb_head == it->skb and 0 == it->frag; so, condition it->frag >= skb_shinfo(it->skb)->nr_frags) probably will be false here and we continue execute all the following tfw_msg_write() calls without warnings.
There was a problem hiding this comment.
The scenario you've described may happen on subsequent tfw_msg_write() calls, which is very common case. So yes, the condition will false and we will continue to execute the function. But *skb == skb_head really means that we run out of the spare place in the skb fragments, and no more data can be pushed there. So if we continue to execute the function, we will walk through all the fragments once again and return -E2BIG error.
So there is no errors and the function can be called multiple times without any worries. The tfw_msg_write() function is used only for preallocated skbs, and data size is known before skb allocations, thus data should perfectly fit the it.
The reason for WARN_ON is invalid it state, where is it->frag points to nonexistent fragment.
| frag = &skb_shinfo(it->skb)->frags[it->frag]; | ||
| unsigned int f_size; | ||
| skb_frag_t *frag = &skb_shinfo(it->skb)->frags[it->frag]; | ||
|
|
There was a problem hiding this comment.
It seems that there is a problem with using skb header when allocating more than one skb - in tfw_http_msg_setup() each skb is allocated with a place under the header:
Line 174 in 5631d72
but in
tfw_msg_write() only the header of the first skb in list is used (it->frag < 0):Line 59 in 5631d72
for subsequent skb
it-> frag is always initialized to zero:Line 132 in 5631d72
so then we always fall into the
it->frag >= 0 branch.Looks like the current master version of
tfw_msg_write() has the same issue.
The function receives TfwMsgIter as argumrnt (as the arguments actually), The 'skb' and 'f' point to specific fragment of the skb. If the condition is false, then TfwMsgIter->skb and TfwMsgIter->frag may point to unexisting skb fragment.
The issue was noticed in comment #1169 (comment) Anyway tfw_apm_update() in tfw_http_hm_drop_resp() doesn't look good.
vankoven
force-pushed
the
ik-fix-uninited-var
branch
from
5631d72
to
f0f07a6
Compare
vankoven
force-pushed
the
ik-fix-uninited-var
branch
from
f0f07a6
to
96b8933
Compare
There was a problem hiding this comment.
Looks good to me (with https://github.com/tempesta-tech/tempesta/pull/1162/files#r252545944 will be fixed in other PR).
Original PR is #1149