Thread: BUG #16190: The usage of NULL pointer in refint.c

BUG #16190: The usage of NULL pointer in refint.c

From
PG Bug reporting form
Date:
The following bug has been logged on the website:

Bug reference:      16190
Logged by:          Jian Zhang
Email address:      starbugs@qq.com
PostgreSQL version: 12.1
Operating system:   Linux
Description:

We checked the code in file “refint.c” and there is one error occurring in
line 636. This error is caused by the usage of pointer with NULL value. The
code in this line is “newp->ident = strdup(ident);” The pointer “newp” is
defined by the code in line 615 as “EPlan *newp;” and initialized by the
code in line 628 as “newp = *eplan + i;” or in line 632 as “newp = *eplan =
(EPlan *) malloc(sizeof(EPlan));” according to different conditions. In the
first condition, the “*eplan” is valued by the code “*eplan = (EPlan *)
realloc(*eplan, (i + 1) * sizeof(EPlan));” in line 627. We found the code
hasn’t checked if the process “realloc” and “malloc” are success or not
which directly define the value of “*eplan”. The program should check the
effectiveness of the return value of function “realloc” and “malloc” to
avoid this error.


Re: BUG #16190: The usage of NULL pointer in refint.c

From
Michael Paquier
Date:
On Mon, Jan 06, 2020 at 03:39:36AM +0000, PG Bug reporting form wrote:
> We checked the code in file “refint.c” and there is one error occurring in
> line 636. This error is caused by the usage of pointer with NULL value. The
> code in this line is “newp->ident = strdup(ident);” The pointer “newp” is
> defined by the code in line 615 as “EPlan *newp;” and initialized by the
> code in line 628 as “newp = *eplan + i;” or in line 632 as “newp = *eplan =
> (EPlan *) malloc(sizeof(EPlan));” according to different conditions. In the
> first condition, the “*eplan” is valued by the code “*eplan = (EPlan *)
> realloc(*eplan, (i + 1) * sizeof(EPlan));” in line 627. We found the code
> hasn’t checked if the process “realloc” and “malloc” are success or not
> which directly define the value of “*eplan”. The program should check the
> effectiveness of the return value of function “realloc” and “malloc” to
> avoid this error.

It could be better to switch all that to not use directly system
calls, and rely properly on a high-level memory context with
palloc-like allocations.  There could be also an argument to just
remove the module per the lack of attention it is getting, though it
is still useful as an example of use for SPI, and the docs mention
it for that.
--
Michael

Attachment

Re: BUG #16190: The usage of NULL pointer in refint.c

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Jan 06, 2020 at 03:39:36AM +0000, PG Bug reporting form wrote:
>> We checked the code in file “refint.c” and there is one error occurring in
>> line 636.

> It could be better to switch all that to not use directly system
> calls, and rely properly on a high-level memory context with
> palloc-like allocations.

Yeah, if somebody wanted to fix this, the right way is to replace
these malloc calls with pallocs.  Some investigation would be needed
about which context to use.

> ... There could be also an argument to just
> remove the module per the lack of attention it is getting, though it
> is still useful as an example of use for SPI, and the docs mention
> it for that.

The regression tests use refint too.  Still, it's not production-grade
code by any means, and I'm not sure if there's much point in making
it so.  I won't stand in the way if somebody else wants to ;-)

            regards, tom lane



Re: BUG #16190: The usage of NULL pointer in refint.c

From
Michael Paquier
Date:
On Mon, Jan 06, 2020 at 01:21:35AM -0500, Tom Lane wrote:
> Yeah, if somebody wanted to fix this, the right way is to replace
> these malloc calls with pallocs.  Some investigation would be needed
> about which context to use.

It seems to me that this would be TopMemoryContext.  All the plans
used for the checks are kept within the context of the session.
--
Michael

Attachment

Re: BUG #16190: The usage of NULL pointer in refint.c

From
Andres Freund
Date:
Hi,

On 2020-01-06 01:21:35 -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > ... There could be also an argument to just
> > remove the module per the lack of attention it is getting, though it
> > is still useful as an example of use for SPI, and the docs mention
> > it for that.
> 
> The regression tests use refint too.  Still, it's not production-grade
> code by any means, and I'm not sure if there's much point in making
> it so.  I won't stand in the way if somebody else wants to ;-)

I think we should consider either moving this out of contrib, or fixing
it up. test/example code is fine, but contrib gets installed by default
for a lot of people... And yea, this isn't just about contrib/spi.

Greetings,

Andres Freund



Re: BUG #16190: The usage of NULL pointer in refint.c

From
Michael Paquier
Date:
On Mon, Jan 06, 2020 at 09:44:43AM -0800, Andres Freund wrote:
> I think we should consider either moving this out of contrib, or fixing
> it up. test/example code is fine, but contrib gets installed by default
> for a lot of people... And yea, this isn't just about contrib/spi.

No idea about moving that out of contrib/, but here is a patch to fix
things that just moves the allocations to TopMemoryContext and removes
the system calls.
--
Michael

Attachment

Re: BUG #16190: The usage of NULL pointer in refint.c

From
Tom Lane
Date:
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Jan 06, 2020 at 09:44:43AM -0800, Andres Freund wrote:
>> I think we should consider either moving this out of contrib, or fixing
>> it up. test/example code is fine, but contrib gets installed by default
>> for a lot of people... And yea, this isn't just about contrib/spi.

> No idea about moving that out of contrib/, but here is a patch to fix
> things that just moves the allocations to TopMemoryContext and removes
> the system calls.

WFM.  There are probably more elegant ways to do it than to drop this
stuff into TopMemoryContext, but this is surely better than unchecked
malloc calls.

            regards, tom lane



Re: BUG #16190: The usage of NULL pointer in refint.c

From
Andres Freund
Date:
On 2020-01-06 21:12:05 -0500, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > On Mon, Jan 06, 2020 at 09:44:43AM -0800, Andres Freund wrote:
> >> I think we should consider either moving this out of contrib, or fixing
> >> it up. test/example code is fine, but contrib gets installed by default
> >> for a lot of people... And yea, this isn't just about contrib/spi.
> 
> > No idea about moving that out of contrib/, but here is a patch to fix
> > things that just moves the allocations to TopMemoryContext and removes
> > the system calls.
> 
> WFM.  There are probably more elegant ways to do it than to drop this
> stuff into TopMemoryContext, but this is surely better than unchecked
> malloc calls.

Yea, it's certainly better than the current situation. An incremental
improvement would be to do the allocations in a separate contect, for easier
debugging should there ever be a leak...

- Andres



Re: BUG #16190: The usage of NULL pointer in refint.c

From
Michael Paquier
Date:
On Mon, Jan 06, 2020 at 06:26:54PM -0800, Andres Freund wrote:
> On 2020-01-06 21:12:05 -0500, Tom Lane wrote:
>> WFM.  There are probably more elegant ways to do it than to drop this
>> stuff into TopMemoryContext, but this is surely better than unchecked
>> malloc calls.
>
> Yea, it's certainly better than the current situation. An incremental
> improvement would be to do the allocations in a separate contect, for easier
> debugging should there ever be a leak...

Sure.  I am not sure if that's worth the extra work though, so I would
just be tempted to commit the patch that moves the allocation to
TopMemoryContext and call it a day.  Any objections to that?
--
Michael

Attachment

Re: BUG #16190: The usage of NULL pointer in refint.c

From
Andres Freund
Date:

On January 6, 2020 10:44:12 PM PST, Michael Paquier <michael@paquier.xyz> wrote:
>On Mon, Jan 06, 2020 at 06:26:54PM -0800, Andres Freund wrote:
>> On 2020-01-06 21:12:05 -0500, Tom Lane wrote:
>>> WFM.  There are probably more elegant ways to do it than to drop
>this
>>> stuff into TopMemoryContext, but this is surely better than
>unchecked
>>> malloc calls.
>>
>> Yea, it's certainly better than the current situation. An incremental
>> improvement would be to do the allocations in a separate contect, for
>easier
>> debugging should there ever be a leak...
>
>Sure.  I am not sure if that's worth the extra work though, so I would
>just be tempted to commit the patch that moves the allocation to
>TopMemoryContext and call it a day.  Any objections to that?

Not from here
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.



Re: BUG #16190: The usage of NULL pointer in refint.c

From
Michael Paquier
Date:
On Mon, Jan 06, 2020 at 10:45:08PM -0800, Andres Freund wrote:
> Not from here

[A couple of days later]
Committed as of b0b6196.
--
Michael

Attachment