Re: WIP Incremental JSON Parser

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