Re: [bug-libunistring] libunistring-0.9.10: coverity scan shows a few more problems in bundled gnulib

[Mike FABIAN]

Bruno Haible <bruno@clisp.org> さんはかきました:

[...]

>> A new scan with Bruno’s patch applied now complains about several more
>> problems. I tried to have a look and as far as I understand it, most of
>> them could be false positives and no real problems. But I am not 100%
>> sure.
>
> libunistring version 0.9.10 is pretty old. Since then, several fixes have
> been applied to vasnprintf.c in particular:
>
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=278b4175c9d7dd47c1a3071554aac02add3b3c35
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=4d288a80bf7ebe29334b9805cdcc70eacb6059c1
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=5527d5c548702b89d217bbe58036996066a709b6
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=54c80fb6f106d7f3430dd075fcb7327bab07f368
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=ca1cd9b39787fe8a2329c77bc60d4a7c3ab2334e
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=c9e246f596836d1eb57120a2d3b65687170356a1
>
> It would therefore be better to do it with libunistring 1.2.

Yes.

> Also, for coverity findings on Gnulib source code, we use the coverity portal 
> where we mark
> false positives etc. - so that when Gnulib source code is used in other 
> packages, redundant
> work does not need to be done.

OK! I’ll try to use that in future!

[...]


[...]

>> Error: OVERRUN (CWE-119):
>> libunistring-0.9.10/lib/uninorm/canonical-decomposition.c:88: cond_at_most: 
>> Checking "entry < 32768" implies that "entry" may be up to 32767 on the true 
>> branch.
>> libunistring-0.9.10/lib/uninorm/canonical-decomposition.c:94: alias:
>> Assigning: "p" = "&gl_uninorm_decomp_chars_table[3 * entry]". "p"
>> may now point to as high as byte 98301 of
>> "gl_uninorm_decomp_chars_table" (which consists of 25575 bytes).
>> libunistring-0.9.10/lib/uninorm/canonical-decomposition.c:95: overrun-local: 
>> Overrunning array of 25575 bytes at byte offset 98301 by dereferencing 
>> pointer "p + 0".
>> #   93|   
>> #   94|             p = &gl_uninorm_decomp_chars_table[3 * entry];
>> #   95|->           element = (p[0] << 16) | (p[1] << 8) | p[2];
>> #   96|             /* The first element has 5 bits for the decomposition 
>> type.  */
>> #   97|             if (((element >> 18) & 0x1f) != UC_DECOMP_CANONICAL)
>> 
>> gl_uninorm_decomp_chars_table is indeed only 25575 bytes long and if entry 
>> can be up to 0x8000-1, then 3*entry could bee too big as an index for 
>> gl_uninorm_decomp_chars_table
>
> We have extensive unit tests of this functionality, and when compiled with
> clang + ASAN, they don't trigger anything.
>
> In summary, while coverity can point to real bugs, nowadays clang + ASAN
> provides a better way (more findings with less effort) to check for mistakes.

So *all* of these errors are false positives. That is very
reassuring. Thank you *very* much for explaining this to me so well!

Yours,

Mike

-- 
Mike FABIAN <mfabian@redhat.com>
睡眠不足はいい仕事の敵だ。