Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)

Em qua., 2 de set. de 2020 às 20:17, Peter Geoghegan <pg(at)bowt(dot)ie> escreveu:

> On Wed, Sep 2, 2020 at 3:16 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:

> Perhaps you recall our discussion of a similar false positive in
> nbtsplitloc.c; that had a similar feel to it. For example, if your
> static analysis tool says that code that has been around for many
> years is riddled with bugs, then the problem is almost certainly with
> the tool (or with how the tool has been used).
>
I'm using about 4 static analysis tools.
Only one of them has a report of 67690 pages.
Assuming an optimistic trend, only 1% could be real failures.
It would have 670 real flaws in the entire code.
In addition, if the code is compiled with -Wextra, hundreds of type
mismatch alerts will be reported,
especially comparison between int (signed) and unsigned types.

If you try to leaks, you will find this:
== 42483 == ERROR: LeakSanitizer: detected memory leaks

Direct leak of 576 byte (s) in 1 object (s) allocated from:
# 0 0x7f3204fb7bc8 in malloc
(/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8)
# 1 0x561c58d5b766 in save_ps_display_args
/usr/src/postgres/src/backend/utils/misc/ps_status.c:178
# 2 0x561c584a90b3 in main /usr/src/postgres/src/backend/main/main.c:89
# 3 0x7f3204b7f0b2 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

Indirect leak of 14 byte (s) in 1 object (s) allocated from:
# 0 0x7f3204f403dd in strdup
(/lib/x86_64-linux-gnu/libasan.so.5+0x963dd)
# 1 0x561c58d5b817 in save_ps_display_args
/usr/src/postgres/src/backend/utils/misc/ps_status.c:186
# 2 0x561c584a90b3 in main /usr/src/postgres/src/backend/main/main.c:89
# 3 0x7f3204b7f0b2 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

For each reported failure, I have analyzed the code, without focusing on
the "intention of the code",
but trying to abstract and concentrate on following the flow of the
variables to make sure that the alert is invalid.
Maybe I didn't realize it, but I'm not "getting out of my head", these
alerts.

>
> High performance C code very often relies on representational
> invariants that may not be readily apparent to a compiler --
> especially when dealing with on-disk state. Barring some obvious
> problem like a hard crash, you have to do the work of assessing if the
> code is correct based on the actual assumptions/context of the code. A
> static analysis tool is of very little use if you're not going to put
> the work in.

I'm sure that all these problems will be solved in the future. But how many
others will be added.
I have realized that maybe, it may be hindering the development course, so
I decided to stop.

I went to a lot of trouble to clearly document the code
> in question here (the heap TID stuff in _bt_truncate()) -- did you
> even bother to read those comments once?
>
Yes, I read.
Maybe in this specific case, only this, it would solve for the code and the
static analysis tool, but it's just a thought.

pivotheaptid = (ItemPointer) ((char *) tidpivot + IndexTupleSize(tidpivot) -
sizeof(ItemPointerData));

Anyway, thanks for all the answers.

Ranier Vilela