[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs

[Alexander Turenko]

On Sun, Apr 28, 2019 at 12:11:28PM +0300, Vladislav Shpilevoy wrote:
> 
> >>
> >>> * xrow_decode_*() functions could read uninitialized data when printing
> >>>   a hex code for a row if the received msgpack is empty, but there are
> >>>   expected keys.
> >>
> >> Please, suggest a change for wording if the current one is not clear.
> 
> Ahh, now I see. It is possible that row->bodycnt == 0 and we jump to
> 'done'. It tries to write an error via xrow_on_decode_err, which reads
> row->body[0]. Probably then I can suggest adding an explicit mention of
> xrow_on_decode_err() function and an attempt to access an array by index
> out of range.
> 
> I did not understand your explanation below about cycles and
> dump_row_hex(), because I did not know, that they are not used in
> xrow_decode_dml() directly, but via xrow_on_decode_err().
> 
> >> My change causes `for (const char *cur = start; cur < end;) { ... }` in
> >> dump_row_hex() to walk around the loop body and so prevents reading from
> >> `start` in the loop (NULL < NULL is false).
> 
> Then you can ignore my comments about xrow_decode_dml and xrow_decode_ballot.
> Others are still valid.

I have updated the comment about the uninitialized data read, added the
comment about getcwd() and the comment about Vinyl:

> Fixed all -Wmaybe-uninitialized and -Wunused-result warnings. A few 
> notes about the fixes:
>
> * net.box does not validate received data in general, so I don't add a
>   check for autoincrement IDs too. Set the ID to INT64_MIN, because this
>   value is less probably will appear here in a normal case and so is the
>   best one to signal a user that something probably going wrongly.
> * xrow_decode_*() functions could read uninitialized data from
>   row->body[0].iov_base in xrow_on_decode_err() when printing a hex code
>   for a row. It could be possible when the received msgpack was empty
>   (row->bodycnt == 0), but there were expected keys (key_map != 0).
> * getcwd() is marked with __attribute__((__warn_unused_result__)) in
>   glibc, but the buffer filled by this call is not used anywhere and so
>   just removed.
> * Vinyl -Wmaybe-uninitialized warnings are false positive ones.

If the patch is okay for you, I'll resend it to Vladimir D. as v2.

> By the way,
> 
>                   \\\ ,
>                     \ `|
>                      ) (   .-""-.
>                      | |  /_  {  '.
>                      | | (/ `\   } )
>                      | |  ^/ ^`}   {
>                      \  \ \=  ( {   )
>                       \  \ '-, {   {{
>                        \  \_.'  ) }  )
>                         \.-'   (     (
>                         /'-.'_. ) (  }
>                         \_(    {   _/\
>                          ) '--' `-;\  \
>                      _.-'       /  / /
>               <\/>_.'         .'  / /
>           <\/></\>/.  '      /<\// /
>           </\>  _ |\`- _ . -/|<// (
>        <\/>    - _- `  _.-'`_/- |  \
>        </\>        -  - -  -     \\\
>         }`<\/>                <\/>`{
>         { </\>-<\/>_<\/>_<\/>-</\> }
>         }      </\> </\> </\>      {
>      <\/>.                         <\/>
>      </\>                          </\>
>       {`<\/>                     <\/>`}
>       } </\>-<\/>_<\/>_<\/>_<\/>-</\> {
>       {      </\> </\> </\> </\>      }
>       }                               }
>       {           H A P P Y           {
>    <\/>        B I R T H D A Y        <\/>
>    </\>                               </\>
>      `<\/>                          <\/>'
>       </\>-<\/>_<\/>_<\/>_<\/>_<\/>-</\>
>            </\> </\> </\> </\> </\>

Thanks!