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
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
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
Attachments:
refint-alloc-fixes.patchtext/x-diff; charset=us-asciiDownload+20-6
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
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, foreasier
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.