Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs
Date: Mon, 29 Apr 2019 00:38:04 +0300	[thread overview]
Message-ID: <20190428213804.gpkzhpz6j3y6drfw@tkn_work_nb> (raw)
In-Reply-To: <b03bc975-3091-bf78-90d8-aa02d2dee9fb@tarantool.org>

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!

  reply	other threads:[~2019-04-28 21:38 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-26  1:29 [tarantool-patches] " Alexander Turenko
2019-04-26 10:18 ` [tarantool-patches] " Vladislav Shpilevoy
2019-04-28  5:40   ` Alexander Turenko
2019-04-28  8:54     ` Vladislav Shpilevoy
2019-04-28  9:11       ` Vladislav Shpilevoy
2019-04-28 21:38         ` Alexander Turenko [this message]
2019-04-28 21:35       ` Alexander Turenko
2019-04-29 10:07         ` Vladislav Shpilevoy
2019-04-29 12:57           ` [PATCH v2] " Alexander Turenko
2019-04-29 17:09             ` Vladimir Davydov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190428213804.gpkzhpz6j3y6drfw@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH] travis-ci: set right flags in release testing jobs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox