Re: WIP Incremental JSON Parser

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.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com