Thread: pgsql: PL/Python: Fix potential NULL pointer dereference
PL/Python: Fix potential NULL pointer dereference After d0aa965c0a0ac2ff7906ae1b1dad50a7952efa56, one error path in PLy_spi_execute_fetch_result() could result in the variable "result" being dereferenced after being set to NULL. To fix that, just clear the resources right there and return early. Also add another SPI_freetuptable() call so that that is cleared in all error paths. discovered by John Naylor <jcnaylor@gmail.com> via scan-build Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/e42e2f38907681c48c43f49c5ec9f9f41a9aa9a5 Modified Files -------------- src/pl/plpython/plpy_spi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Peter Eisentraut <peter_e@gmx.net> writes:
> PL/Python: Fix potential NULL pointer dereference
I do not think it's correct to just "return" out of the middle
of a PG_TRY block --- doesn't that result in failing to pop the
PG_exception_stack and error_context_stack?
regards, tom lane
On 11/28/17 11:35, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: >> PL/Python: Fix potential NULL pointer dereference > > I do not think it's correct to just "return" out of the middle > of a PG_TRY block --- doesn't that result in failing to pop the > PG_exception_stack and error_context_stack? OK, I'll revert and rethink. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 11/28/17 13:35, Peter Eisentraut wrote: > On 11/28/17 11:35, Tom Lane wrote: >> Peter Eisentraut <peter_e@gmx.net> writes: >>> PL/Python: Fix potential NULL pointer dereference >> >> I do not think it's correct to just "return" out of the middle >> of a PG_TRY block --- doesn't that result in failing to pop the >> PG_exception_stack and error_context_stack? > > OK, I'll revert and rethink. How about this instead? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 11/28/17 13:35, Peter Eisentraut wrote:
>> OK, I'll revert and rethink.
> How about this instead?
This looks better, but I think there are a couple more things:
* Seems like you ought to s/Py_DECREF(result)/Py_XDECREF(result)/ in the
PG_CATCH block (line 451 in HEAD). As this is coded, although result
is set to NULL within the PG_TRY, it doesn't seem possible for control
to reach PG_CATCH after that ... but that's a pretty fragile assumption.
* You'd also need to declare "result" as
PLyResultObject *volatile result;
if you intend to change it within the PG_TRY block.
On the whole it seems like it might be better to dodge this whole business
of changing "result" inside the TRY. You could do that if you did
something like
result->rows = PyList_New(rows);
- if (!result->rows)
- {
- Py_DECREF(result);
- result = NULL;
- }
- else
+ if (result->rows)
{
PLy_input_setup_tuple(&ininfo, tuptable->tupdesc,
and then add after the PG_END_TRY
if (!result->rows)
{
Py_DECREF(result);
result = NULL;
}
The repeated test is a bit annoying but maybe it's better than the
alternative. You'd probably need a comment explaining why it's
like that, though.
regards, tom lane
On 12/5/17 14:45, Tom Lane wrote:
> On the whole it seems like it might be better to dodge this whole business
> of changing "result" inside the TRY. You could do that if you did
> something like
>
> result->rows = PyList_New(rows);
> - if (!result->rows)
> - {
> - Py_DECREF(result);
> - result = NULL;
> - }
> - else
> + if (result->rows)
> {
> PLy_input_setup_tuple(&ininfo, tuptable->tupdesc,
>
> and then add after the PG_END_TRY
>
> if (!result->rows)
> {
> Py_DECREF(result);
> result = NULL;
> }
Yeah, that looks much better. Next try attached.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 12/5/17 14:45, Tom Lane wrote:
>> On the whole it seems like it might be better to dodge this whole business
>> of changing "result" inside the TRY. You could do that if you did
>> something like ...
> Yeah, that looks much better. Next try attached.
Looks sane to me.
regards, tom lane