Thread: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
[NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Ranier Vilela
Date:
Hi,
Is possible that BTreeTupleSetNAtts, leave everything tidy, so that BTreeTupleGetHeapTID doesn't fail.
BTreeTupleGetHeapTID can return NULL.
But, as we can see:
1. Line 2085 (nbtutils.c):
if (BTreeTupleGetHeapTID(itup) != NULL && tupnatts != nkeyatts)
2. Line 803 (nbtsearch.c):
if (heapTid == NULL)
Maybe, better make sure, because:
3. Line 2285 (nbtutils.c):
ItemPointerCopy(BTreeTupleGetMaxHeapTID(lastleft), pivotheaptid);
4. Line 2316 (nbtutils.c) :
ItemPointerCopy(BTreeTupleGetHeapTID(firstright), pivotheaptid);
Can dereference NULL pointer (pivotheaptid) at runtime (release version).
itemptr.h:
#define ItemPointerCopy(fromPointer, toPointer) \
( \
AssertMacro(PointerIsValid(toPointer)), \
AssertMacro(PointerIsValid(fromPointer)), \
*(toPointer) = *(fromPointer) \
)
( \
AssertMacro(PointerIsValid(toPointer)), \
AssertMacro(PointerIsValid(fromPointer)), \
*(toPointer) = *(fromPointer) \
)
regards,
Ranier Vilela
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Ranier Vilela
Date:
Also, in:
5. Line 2671 (nbtutils.c):
ItemPointerGetBlockNumber(BTreeTupleGetHeapTID(newtup)),
ItemPointerGetOffsetNumber(BTreeTupleGetHeapTID(newtup)),
ItemPointerGetOffsetNumber(BTreeTupleGetHeapTID(newtup)),
itemptr.h:
/*
* ItemPointerGetBlockNumberNoCheck
* Returns the block number of a disk item pointer.
*/
#define ItemPointerGetBlockNumberNoCheck(pointer) \
( \
BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
)
* ItemPointerGetBlockNumberNoCheck
* Returns the block number of a disk item pointer.
*/
#define ItemPointerGetBlockNumberNoCheck(pointer) \
( \
BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
)
#define ItemPointerGetOffsetNumberNoCheck(pointer) \
( \
(pointer)->ip_posid \
)
( \
(pointer)->ip_posid \
)
regards,
Ranier VilelaRe: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Peter Geoghegan
Date:
On Wed, Sep 2, 2020 at 2:41 PM Ranier Vilela <ranier.vf@gmail.com> wrote: > Maybe, better make sure, because: > 3. Line 2285 (nbtutils.c): > ItemPointerCopy(BTreeTupleGetMaxHeapTID(lastleft), pivotheaptid); > 4. Line 2316 (nbtutils.c) : > ItemPointerCopy(BTreeTupleGetHeapTID(firstright), pivotheaptid); > > Can dereference NULL pointer (pivotheaptid) at runtime (release version). The entire codepath in question exists to set a new pivot tuple's heap TID, in the case where we have to include a heap TID in a new leaf page high key. This is a tuple in palloc()'d memory that we ourselves just created. We know that BTreeTupleGetHeapTID() will return a valid heap TID pointer (a pointer into the end of the new pivot tuple buffer) because we just marked the pivot tuple as having space for one ourselves -- we still completely own the tuple. While it's true that in general BTreeTupleGetHeapTID() can return a NULL pointer, it does not matter here. Even if BTreeTupleGetHeapTID() did somehow return a NULL pointer, then the user would be getting off lightly by experiencing a hard crash instead of data corruption. You should spend more time (as in more than zero time) trying to understand the intent of the code that you write these reports about. -- Peter Geoghegan
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Ranier Vilela
Date:
Even if BTreeTupleGetHeapTID() did somehow return a NULL
pointer, then the user would be getting off lightly by experiencing a
hard crash instead of data corruption.
Oh, I'm sorry, I thought that "hard crash" was a bad thing.
Ranier Vilela
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Peter Geoghegan
Date:
On Wed, Sep 2, 2020 at 3:16 PM Ranier Vilela <ranier.vf@gmail.com> wrote: > Oh, I'm sorry, I thought that "hard crash" was a bad thing. I think that you are being sarcastic here, but just in case I'm wrong I'll clarify what I meant: a good sign that a static analysis tool has produced a useless complaint is that it appears to prove something far stronger than you first thought possible. You should look out for that. This is just a heuristic, but in practice it is a good one. 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). 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 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? -- Peter Geoghegan
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Ranier Vilela
Date:
Em qua., 2 de set. de 2020 às 20:17, Peter Geoghegan <pg@bowt.ie> escreveu:
On Wed, Sep 2, 2020 at 3:16 PM Ranier Vilela <ranier.vf@gmail.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.
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,
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",
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.
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 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));
sizeof(ItemPointerData));
Ranier Vilela
Re: [NBTREE] Possible NULL pointer dereference (backend/access/nbtree/nbutils.c)
From
Michael Paquier
Date:
On Thu, Sep 03, 2020 at 11:39:57AM -0300, Ranier Vilela wrote: > I'm using about 4 static analysis tools. It seems to me that this is the root of the problem here. -- Michael