[PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Started by Ranier Vilelaover 5 years ago13 messages
#1Ranier Vilela
ranier.vf@gmail.com
1 attachment(s)

Hi Tom,

Per Coverity.

The variable root_offsets is read at line 1641, but, at this point,
the content is unknown, so it is impossible to test works well.

regards,
Ranier Vilela

Attachments:

fix_uninitialized_variable_heapam_handler.patchapplication/octet-stream; name=fix_uninitialized_variable_heapam_handler.patchDownload
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index dcaea7135f..64385d56d1 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1285,6 +1285,8 @@ heapam_index_build_range_scan(Relation heapRelation,
 	}
 
 	reltuples = 0;
+	MemSet(root_offsets, InvalidOffsetNumber,
+		   MaxHeapTuplesPerPage);
 
 	/*
 	 * Scan all tuples in the base relation.
#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#1)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

On 2020-Aug-25, Ranier Vilela wrote:

The variable root_offsets is read at line 1641, but, at this point,
the content is unknown, so it is impossible to test works well.

Surely it is set by heap_get_root_tuples() in line 1347? The root_blkno
variable is used exclusively to know whether root_offsets has been
initialized for the current block.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#3Ranier Vilela
ranier.vf@gmail.com
In reply to: Alvaro Herrera (#2)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Em ter., 25 de ago. de 2020 às 18:06, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

The variable root_offsets is read at line 1641, but, at this point,
the content is unknown, so it is impossible to test works well.

Surely it is set by heap_get_root_tuples() in line 1347? The root_blkno
variable is used exclusively to know whether root_offsets has been
initialized for the current block.

Hi Álvaro,

20. Condition hscan->rs_cblock != root_blkno, taking false branch.

If the variable hscan->rs_cblock is InvalidBlockNumber the test can
protect root_offsets fail.

The var root_blkno only is checked at line 1853.

regards,
Ranier Vilela

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#3)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

On 2020-Aug-25, Ranier Vilela wrote:

If the variable hscan->rs_cblock is InvalidBlockNumber the test can
protect root_offsets fail.

When does that happen?

The var root_blkno only is checked at line 1853.

That's a different function.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#5Ranier Vilela
ranier.vf@gmail.com
In reply to: Alvaro Herrera (#4)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Em ter., 25 de ago. de 2020 às 19:45, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

If the variable hscan->rs_cblock is InvalidBlockNumber the test can
protect root_offsets fail.

When does that happen?

At first pass into the while loop?
hscan->rs_cblock is InvalidBlockNumber, what happens?

The var root_blkno only is checked at line 1853.

That's a different function.

My mistake. Find editor.

regards,
Ranier Vilela

#6Ranier Vilela
ranier.vf@gmail.com
In reply to: Ranier Vilela (#5)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Em ter., 25 de ago. de 2020 às 19:54, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

Em ter., 25 de ago. de 2020 às 19:45, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

If the variable hscan->rs_cblock is InvalidBlockNumber the test can
protect root_offsets fail.

When does that happen?

At first pass into the while loop?
hscan->rs_cblock is InvalidBlockNumber, what happens?

Other things.

1. Even heap_get_root_tuples at line 1347, be called.
Does it fill all roots_offsets?
root_offsets[offnum - 1] is secure at this point (line 1641 or is trash)?

2. hscan->rs_cbuf is constant?
if (hscan->rs_cblock != root_blkno)
{
Page page = BufferGetPage(hscan->rs_cbuf);

LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_SHARE);
heap_get_root_tuples(page, root_offsets);
LockBuffer(hscan->rs_cbuf, BUFFER_LOCK_UNLOCK);

root_blkno = hscan->rs_cblock;
}

This can move outside while loop?
Am I wrong or hscan do not change?

regards,
Ranier Vilela

#7Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#5)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

On 2020-Aug-25, Ranier Vilela wrote:

Em ter., 25 de ago. de 2020 �s 19:45, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

If the variable hscan->rs_cblock is InvalidBlockNumber the test can
protect root_offsets fail.

When does that happen?

At first pass into the while loop?
hscan->rs_cblock is InvalidBlockNumber, what happens?

No, it is set when the page is read.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#8Ranier Vilela
ranier.vf@gmail.com
In reply to: Alvaro Herrera (#7)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Em ter., 25 de ago. de 2020 às 20:13, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

Em ter., 25 de ago. de 2020 às 19:45, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

If the variable hscan->rs_cblock is InvalidBlockNumber the test can
protect root_offsets fail.

When does that happen?

At first pass into the while loop?
hscan->rs_cblock is InvalidBlockNumber, what happens?

No, it is set when the page is read.

And it is guaranteed that, rs_cblock is not InvalidBlockNumber when the
page is read?

Ranier Vilela

#9Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#6)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

On 2020-Aug-25, Ranier Vilela wrote:

1. Even heap_get_root_tuples at line 1347, be called.
Does it fill all roots_offsets?

Yes -- read the comments there.

2. hscan->rs_cbuf is constant?
if (hscan->rs_cblock != root_blkno)

It is the buffer that contains the given block. Those two things move
in unison. See heapgettup and heapgettup_pagemode.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#10Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#8)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

On 2020-Aug-25, Ranier Vilela wrote:

And it is guaranteed that, rs_cblock is not InvalidBlockNumber when the
page is read?

It could be InvalidBlockNumber if sufficient neutrinos hit the memory
bank and happen to set all the bits in the block number.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#11Ranier Vilela
ranier.vf@gmail.com
In reply to: Alvaro Herrera (#10)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Em ter., 25 de ago. de 2020 às 20:20, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

And it is guaranteed that, rs_cblock is not InvalidBlockNumber when the
page is read?

It could be InvalidBlockNumber if sufficient neutrinos hit the memory
bank and happen to set all the bits in the block number.

kkk, I think it's enough for me.
I believe that PostgreSQL will not run on the ISS yet.

Ranier Vilela

#12Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#11)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

On 2020-Aug-25, Ranier Vilela wrote:

kkk, I think it's enough for me.
I believe that PostgreSQL will not run on the ISS yet.

Actually, I believe there are some satellites that run Postgres -- not
100% sure about this.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#13Ranier Vilela
ranier.vf@gmail.com
In reply to: Alvaro Herrera (#12)
Re: [PATCH] Fix Uninitialized scalar variable (UNINIT) (src/backend/access/heap/heapam_handler.c)

Em ter., 25 de ago. de 2020 às 20:29, Alvaro Herrera <
alvherre@2ndquadrant.com> escreveu:

On 2020-Aug-25, Ranier Vilela wrote:

kkk, I think it's enough for me.
I believe that PostgreSQL will not run on the ISS yet.

Actually, I believe there are some satellites that run Postgres -- not
100% sure about this.

Yeah, ESA uses:
https://resources.2ndquadrant.com/european-space-agency-case-study-download

In fact, Postgres is to be congratulated.
Guess who didn't make any bug?
https://changochen.github.io/publication/squirrel_ccs2020.pdf
"Sqirrel has successfully detected 63 bugs from tested DBMS,including 51
bugs from SQLite, 7 from MySQL, and 5 from MariaDB."

Ranier Vilela