On 2024-04-08 Mo 09:29, Andrew Dunstan wrote:
>
> On 2024-04-07 Su 20:58, Tom Lane wrote:
>> Coverity complained that this patch leaks memory:
>>
>> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_combinebackup/load_manifest.c:
>> 212 in load_backup_manifest()
>> 206 bytes_left -= rc;
>> 207 json_parse_manifest_incremental_chunk(
>> 208 inc_state, buffer, rc, bytes_left == 0);
>> 209 }
>> 210
>> 211 close(fd);
>>>>> CID 1596259: (RESOURCE_LEAK)
>>>>> Variable "inc_state" going out of scope leaks the storage it
>>>>> points to.
>> 212 }
>> 213
>> 214 /* All done. */
>> 215 pfree(buffer);
>> 216 return result;
>> 217 }
>>
>> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:
>> 488 in parse_manifest_file()
>> 482 bytes_left -= rc;
>> 483 json_parse_manifest_incremental_chunk(
>> 484 inc_state, buffer, rc, bytes_left == 0);
>> 485 }
>> 486
>> 487 close(fd);
>>>>> CID 1596257: (RESOURCE_LEAK)
>>>>> Variable "inc_state" going out of scope leaks the storage it
>>>>> points to.
>> 488 }
>> 489
>> 490 /* Done with the buffer. */
>> 491 pfree(buffer);
>> 492
>> 493 return result;
>>
>> It's right about that AFAICS, and not only is the "inc_state" itself
>> leaked but so is its assorted infrastructure. Perhaps we don't care
>> too much about that in the existing applications, but ISTM that
>> isn't going to be a tenable assumption across the board. Shouldn't
>> there be a "json_parse_manifest_incremental_shutdown()" or the like
>> to deallocate all the storage allocated by the parser?
>
>
>
> yeah, probably. Will work on it.
>
>
>
Here's a patch. In addition to the leaks Coverity found, there was
another site in the backend code that should call the shutdown function,
and a probably memory leak from a logic bug in the incremental json
parser code. All these are fixed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com