Thread: BUG #16190: The usage of NULL pointer in refint.c
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.
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
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
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
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
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
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
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
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
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.
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