Re: possible use of unitialized value in v2.0-dev0-274-g1a0fe3be
On Wed, 6 Feb 2019 19:12:31 +0100
Tim Düsterhus wrote:
> Line 4398 is missing here, it appends a marker (empty string) to mark
> the end of the array.
>
> > ...
> >
> > 4450 /* look for the Host header and place it in :authority
> > */
> > 4451 auth = ist2(NULL, 0);
> > 4452 for (hdr = 0; hdr < sizeof(list)/sizeof(list[0]);
> > hdr++) {
> > 4453 if (isteq(list[hdr].n, ist("")))
> > // (here, assume the condition is false, so control keeps in this block...)
> >
>
> We established that `list` is an array without holes terminated by an
> empty string.
>
> Thus either:
> 1. The Condition is false, then the value must be initialized
> or
> 2. The Condition is true, then the loop is exited.
>
> Thus I believe this is a false-positive.
Thank you for checking this out and, by extension, Willy on his set of
replise. I missed the marker (and apparently, so did Clang).
Cheers,
--
Ricardo Nabinger Sanchez http://www.taghos.com.br/
Re: possible use of unitialized value in v2.0-dev0-274-g1a0fe3be
On Wed, Feb 06, 2019 at 07:12:31PM +0100, Tim Düsterhus wrote: (...) > Thus I believe this is a false-positive. I should have read the whole thread, it would have saved me a reply :-) Willy
Re: possible use of unitialized value in v2.0-dev0-274-g1a0fe3be
Hello,
On Wed, Feb 06, 2019 at 02:28:27PM -0200, Ricardo Nabinger Sanchez wrote:
> Hello,
>
> scan-build found a 28-step path where an unitialized value could be used in
> h2s_htx_bck_make_req_headers().
>
> Here is a shortened version:
>
> 4378 idx = htx_get_head(htx); // returns the SL that we skip
> 4379 while ((idx = htx_get_next(htx, idx)) != -1) {
> 4380 blk = htx_get_blk(htx, idx);
> 4381 type = htx_get_blk_type(blk);
> 4382
> 4383 if (type == HTX_BLK_UNUSED)
> 4384 continue;
> 4385
> 4386 if (type != HTX_BLK_HDR)
> // (here, assume condition is true, so control leaves the loop...)
> 4387 break;
> 4388
> 4389 if (unlikely(hdr >= sizeof(list)/sizeof(list[0]) - 1))
> 4390 goto fail;
> 4391
> // (... and list will not be initialized.)
> 4392 list[hdr].n = htx_get_blk_name(htx, blk);
> 4393 list[hdr].v = htx_get_blk_value(htx, blk);
> 4394 hdr++;
> 4395 }
>
> ...
The most important piece was skipped here :
/* marker for end of headers */
list[hdr].n = ist("");
So the list is *always* terminated.
> 4450 /* look for the Host header and place it in :authority */
> 4451 auth = ist2(NULL, 0);
> 4452 for (hdr = 0; hdr < sizeof(list)/sizeof(list[0]); hdr++)
> {
> 4453 if (isteq(list[hdr].n, ist("")))
> // (here, assume the condition is false, so control keeps in this block...)
> 4454 break; // end
Here the only way to reach this point is to have met an initialized
element, since the first one after the last initialized one sets the
name to the empty string and causes the break to happen.
> 4455
> 4456 if (isteq(list[hdr].n, ist("host"))) {
> 4457 auth = list[hdr].v;
> // (... auth receives an uninitialized value from list ...)
Thus no by definition.
> 4458 break;
> 4459 }
> 4460 }
> 4461 }
(...)
> While this feels like a convoluted or unlikely scenario, the path leading
> to the use of uninitialized value seems to be correctly unearthed by
> scan-build.
Fortunately no, it's a classical false positive instead.
Thanks!
Willy
Re: possible use of unitialized value in v2.0-dev0-274-g1a0fe3be
Ricardo,
Am 06.02.19 um 17:28 schrieb Ricardo Nabinger Sanchez:
> Hello,
>
> scan-build found a 28-step path where an unitialized value could be used in
> h2s_htx_bck_make_req_headers().
>
> Here is a shortened version:
>
> 4378 idx = htx_get_head(htx); // returns the SL that we skip
> 4379 while ((idx = htx_get_next(htx, idx)) != -1) {
> 4380 blk = htx_get_blk(htx, idx);
> 4381 type = htx_get_blk_type(blk);
> 4382
> 4383 if (type == HTX_BLK_UNUSED)
> 4384 continue;
> 4385
> 4386 if (type != HTX_BLK_HDR)
> // (here, assume condition is true, so control leaves the loop...)
> 4387 break;
> 4388
> 4389 if (unlikely(hdr >= sizeof(list)/sizeof(list[0]) - 1))
> 4390 goto fail;
> 4391
> // (... and list will not be initialized.)
Yes, but hdr will not be incremented either. Thus `list` is an array
without holes.
> 4392 list[hdr].n = htx_get_blk_name(htx, blk);
> 4393 list[hdr].v = htx_get_blk_value(htx, blk);
> 4394 hdr++;
> 4395 }
Line 4398 is missing here, it appends a marker (empty string) to mark
the end of the array.
> ...
>
> 4450 /* look for the Host header and place it in :authority */
> 4451 auth = ist2(NULL, 0);
> 4452 for (hdr = 0; hdr < sizeof(list)/sizeof(list[0]); hdr++)
> {
> 4453 if (isteq(list[hdr].n, ist("")))
> // (here, assume the condition is false, so control keeps in this block...)
We established that `list` is an array without holes terminated by an
empty string.
Thus either:
1. The Condition is false, then the value must be initialized
or
2. The Condition is true, then the loop is exited.
Thus I believe this is a false-positive.
Best regards
Tim Düsterhus
possible use of unitialized value in v2.0-dev0-274-g1a0fe3be
Hello,
scan-build found a 28-step path where an unitialized value could be used in
h2s_htx_bck_make_req_headers().
Here is a shortened version:
4378 idx = htx_get_head(htx); // returns the SL that we skip
4379 while ((idx = htx_get_next(htx, idx)) != -1) {
4380 blk = htx_get_blk(htx, idx);
4381 type = htx_get_blk_type(blk);
4382
4383 if (type == HTX_BLK_UNUSED)
4384 continue;
4385
4386 if (type != HTX_BLK_HDR)
// (here, assume condition is true, so control leaves the loop...)
4387 break;
4388
4389 if (unlikely(hdr >= sizeof(list)/sizeof(list[0]) - 1))
4390 goto fail;
4391
// (... and list will not be initialized.)
4392 list[hdr].n = htx_get_blk_name(htx, blk);
4393 list[hdr].v = htx_get_blk_value(htx, blk);
4394 hdr++;
4395 }
...
4450 /* look for the Host header and place it in :authority */
4451 auth = ist2(NULL, 0);
4452 for (hdr = 0; hdr < sizeof(list)/sizeof(list[0]); hdr++) {
4453 if (isteq(list[hdr].n, ist("")))
// (here, assume the condition is false, so control keeps in this block...)
4454 break; // end
4455
4456 if (isteq(list[hdr].n, ist("host"))) {
4457 auth = list[hdr].v;
// (... auth receives an uninitialized value from list ...)
4458 break;
4459 }
4460 }
4461 }
4462 else {
4463 /* for CONNECT, :authority is taken from the path */
4464 auth = path;
4465 }
4466
// (... and here auth is evaluated, but it contains whatever uninitialized
// data that list had, because its initialization has been jumped over.)
4467 if (auth.ptr && !hpack_encode_header(, ist(":authority"),
auth)) {
4468 /* output full */
4469 if (b_space_wraps(>mbuf))
4470 goto realign_again;
4471 goto full;
4472 }
While this feels like a convoluted or unlikely scenario, the path leading
to the use of uninitialized value seems to be correctly unearthed by
scan-build. Also, there might be a chance that this path invokes undefined
behavior, leading to further surprises.
Does this make sense?
Cheers,
--
Ricardo Nabinger Sanchez http://www.taghos.com.br/
