Fix PGresult leak in PQclosePrepared cleanup path (libpq >= 17)

[rorsarach]

In pg_st_deallocate_statement(), free the existing sth result before calling PQclosePrepared(). #187

[esabol]

@rorsarach : The CI tests are failing because of Perl Critic things. Please fix. Make sure you setenv RELEASE_TESTING=1 and AUTHOR_TESTING=1 when you run the tests locally.

I suspect @turnstep will want some other changes to your test script as well in order to fit in better with the project's testing framework. EDIT: Actually, this looks pretty close to the style of most of the project's test files, so maybe not. Good job!

[esabol]

@rorsarach : It is still failing a test in t/99_lint.t. Please revise. Make sure you setenv RELEASE_TESTING=1 and AUTHOR_TESTING=1 when you run the tests locally.

[esabol]

All CI tests pass now. Thank you, @rorsarach. And, for the record, I have confirmed that the test script fails on DBD::Pg 3.20.2.

[turnstep]

Thanks for catching this and writing some great tests! We'll get this sorted out and fixed soon.

[turnstep]

I'm a little confused by the logic here. If they ARE equal, we only clear imp_dbh, but if they are NOT equal, we only clear imp_sth? Maybe we want to do something like the other places where we clear before set, like line 3810?

[rorsarach]

Thanks for flagging that this is confusing. The intent was to avoid calling PQclear twice on the same PGresult: imp_sth->result and imp_dbh->last_result often point to the same object returned from a single PQexecPrepared (the execute path assigns them together as imp_dbh->last_result = imp_sth->result = …). When they are equal, we should call PQclear only once, null out both pointers, and only then hand off to PQclosePrepared to set the new result.

When they are not equal, the current code only frees imp_sth->result. From an ownership perspective, the subsequent imp_dbh->last_result = imp_sth->result = PQclosePrepared(...) overwrites whatever last_result was pointing at on the connection. If last_result still referred to a different, not-yet-freed PGresult, there is in principle the same kind of risk as “clear the sth side but not the last_result side.”

[esabol]

@turnstep : Have you had the change to look over @rorsarach 's most recent commit? c52ccac It simplifies the logic somewhat.

OK to merge?

[turnstep]

We always want to clear sth, we all agree on that. But there are times when we do NOT want to clear dbh. That's why there needs to be a result_clearable check. If result_clearable is false, that means that something else is also looking at that result (e.g. another sth), so we cannot free it (unless that someone else is us!) But if it's not ours, it is safe to move the imp_dbh->last_result pointer without freeing it, as it's Somebody Else's Problem. So the code becomes this:

if (imp_sth->result) {
  TRACE_PQCLEAR;
  PQclear(imp_sth->result);
}
/* If last_result was from us, ignore result_clearable and set last_result to null */
if (imp_dbh->last_result == imp_sth->result) {
  imp_dbh->last_result = NULL;
}
/* Otherwise, free it unless somebody else has asked us not to */
else if (imp_dbh->last_result && imp_dbh->result_clearable) {
  TRACE_PQCLEAR;
  PQclear(imp_dbh->last_result);
  imp_dbh->last_result = NULL;
}
imp_sth->result = NULL;

Took me a little bit to wrap my head around, but I think the above is sound. If something else comes in and sets a new last_result after we do, then we no longer match. We may or may not clear it, depending on if that other process ceded control, but either way, we are going to replace the last_result pointer. Technically, those NULL settings are not needed, but it's good practice.

[turnstep]

Obviously we will have to clean up all the places we do this to make sure it all works the same way (not in this PR)

[esabol]

@rorsarach : Would you be willing to change your changes to dbdimp.c to look like the code in @turnstep's post above here?

[rorsarach]

Sorry, I didn't understand how the code should be modified, based on the chat I thought this piece of code didn't need me to modify it.
Is this code based on this modification?

`
if (imp_sth->result) {
TRACE_PQCLEAR;
PQclear(imp_sth->result);
}

/* If last_result was from us, ignore result_clearable and set last_result to null */
if (imp_dbh->last_result == imp_sth->result) {
imp_dbh->last_result = NULL;
}

/* Otherwise, free it unless somebody else has asked us not to */
else if (imp_dbh->last_result && imp_dbh->result_clearable) {
TRACE_PQCLEAR;
PQclear(imp_dbh->last_result);
imp_dbh->last_result = NULL;
}

imp_sth->result = NULL;
`

[rorsarach]

Already done in commit c52ccac. The if/else block has been replaced with two independent checks — first clearing imp_sth->result only when it differs from imp_dbh->last_result (to avoid double-free), then unconditionally clearing and nulling imp_dbh->last_result, and finally nulling imp_sth->result. This matches the clear-before-set pattern @turnstep suggested.

[turnstep]

You still need to be checking imp_dbh->result_clearable

[rorsarach]

Done in commit 0d9016c. The cleanup logic has been revised to match the two-step pattern used in dbd_st_execute (around line 3811):

Free last_result only if it is independently owned (result_clearable is true).
Free sth->result unconditionally.
Why the original code was wrong:

The original if/else had two problems.

First, the guard imp_sth->result != imp_dbh->last_result prevented freeing in the common case. After PQexecPrepared, both pointers share the same PGresult and result_clearable is FALSE. With the guard, neither branch fired, so the PGresult leaked on every prepare/execute/destroy cycle. This is confirmed by t/13leak.t (RSS grows ~37 MB over 1000 cycles on a patched build without the fix).

Second, last_result was freed unconditionally without checking result_clearable. If a second statement (sth_b) had executed after sth_a, last_result would point to sth_b's PGresult with result_clearable = FALSE. Destroying sth_a would then free sth_b's result early, leaving sth_b with a dangling pointer.

How the new code handles each case:

Common case (last_result == sth->result, result_clearable = FALSE): step 1 is skipped; step 2 frees the shared PGresult exactly once.
last_result independently owned (result_clearable = TRUE, different pointer): step 1 frees last_result; step 2 frees sth->result; one free each, no double-free.
Another sth's result in last_result (result_clearable = FALSE, different pointer): step 1 is skipped (protecting the other sth's result); step 2 frees only this sth's result.

[esabol]

@rorsarach : The CI tests are suddenly failing. Not sure why. Try rebasing. You'll need to revise your changes to the Changes file again, sorry.

[rorsarach]

Addressed @turnstep's unresolved review comments:

1. dbdimp.c cleanup logic (inline comment at the if/else block): Good catch — the original if/else had a subtle gap: when imp_sth->result != imp_dbh->last_result, the old imp_dbh->last_result was never freed before being overwritten (potential leak). Simplified it to the clear-before-set pattern like line 3810:

• Clear imp_sth->result only if it differs from imp_dbh->last_result (avoids double-free when they're the same pointer)
• Unconditionally clear and null imp_dbh->last_result
• Null imp_sth->result
  This handles all pointer combinations cleanly.

2. Table name (comment on t/13pqclose_leak.t line 32): The table has always been named dbd_pg_pqclose_leak_test, which already starts with dbd_pg_. No change needed.

3. Single test file (comment on t/13leak.t line 3): Addressed by renaming to /13leak.t as a generic memory/leak framework, per your own suggestion ('Or maybe just 13leak.t to keep with the single word paradigm').

4. RSS growth test in CI forever (comment on t/13leak.t line 218): I agree with @esabol — the test is fast, prevents future regressions, and provides a reusable framework for future leak tests. I'd like to keep it.

5. line 5559 standardization (comment on dbdimp.c line 4221): This points at the imp_sth->result != imp_dbh->last_result guard in the existing async result-store path (lines 5576-5579 of current master). That is pre-existing code outside the scope of this PR. Happy to submit a separate follow-up PR to address it if helpful.

[Edited by @esabol to fix formatting.]

[rorsarach]

Simplify PQclosePrepared result cleanup in pg_st_deallocate_statement

[rorsarach]

Thanks for flagging that this is confusing. The intent was to avoid calling PQclear twice on the same PGresult: imp_sth->result and imp_dbh->last_result often point to the same object returned from a single PQexecPrepared (the execute path assigns them together as imp_dbh->last_result = imp_sth->result = …). When they are equal, we should call PQclear only once, null out both pointers, and only then hand off to PQclosePrepared to set the new result.

When they are not equal, the current code only frees imp_sth->result. From an ownership perspective, the subsequent imp_dbh->last_result = imp_sth->result = PQclosePrepared(...) overwrites whatever last_result was pointing at on the connection. If last_result still referred to a different, not-yet-freed PGresult, there is in principle the same kind of risk as “clear the sth side but not the last_result side.”