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
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 131598bade..8ae4a62ccc 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -241,6 +241,9 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib)
pfree(ib->buf.data);
ib->buf.data = NULL;
+ /* Done with inc_state, so release that memory too */
+ json_parse_manifest_incremental_shutdown(ib->inc_state);
+
/* Switch back to previous memory context. */
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c
index 9c9332cdd5..d857ea0006 100644
--- a/src/bin/pg_combinebackup/load_manifest.c
+++ b/src/bin/pg_combinebackup/load_manifest.c
@@ -208,6 +208,9 @@ load_backup_manifest(char *backup_directory)
inc_state, buffer, rc, bytes_left == 0);
}
+ /* Release the incremental state memory */
+ json_parse_manifest_incremental_shutdown(inc_state);
+
close(fd);
}
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 90ef4b2037..9594c615c7 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -484,6 +484,9 @@ parse_manifest_file(char *manifest_path)
inc_state, buffer, rc, bytes_left == 0);
}
+ /* Release the incremental state memory */
+ json_parse_manifest_incremental_shutdown(inc_state);
+
close(fd);
}
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 44dbb7f7f9..9dfbc397c0 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -488,19 +488,18 @@ freeJsonLexContext(JsonLexContext *lex)
if (lex->errormsg)
destroyStringInfo(lex->errormsg);
+ if (lex->incremental)
+ {
+ pfree(lex->inc_state->partial_token.data);
+ pfree(lex->inc_state);
+ pfree(lex->pstack->prediction);
+ pfree(lex->pstack->fnames);
+ pfree(lex->pstack->fnull);
+ pfree(lex->pstack);
+ }
+
if (lex->flags & JSONLEX_FREE_STRUCT)
- {
- if (lex->incremental)
- {
- pfree(lex->inc_state->partial_token.data);
- pfree(lex->inc_state);
- pfree(lex->pstack->prediction);
- pfree(lex->pstack->fnames);
- pfree(lex->pstack->fnull);
- pfree(lex->pstack);
- }
pfree(lex);
- }
}
/*
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 970a756ce8..a94e3d6b15 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -123,7 +123,6 @@ static bool parse_xlogrecptr(XLogRecPtr *result, char *input);
/*
* Set up for incremental parsing of the manifest.
- *
*/
JsonManifestParseIncrementalState *
@@ -163,6 +162,18 @@ json_parse_manifest_incremental_init(JsonManifestParseContext *context)
return incstate;
}
+/*
+ * Free an incremental state object and its contents.
+ */
+void
+json_parse_manifest_incremental_shutdown(JsonManifestParseIncrementalState *incstate)
+{
+ pfree(incstate->sem.semstate);
+ freeJsonLexContext(&(incstate->lex));
+ /* incstate->manifest_ctx has already been freed */
+ pfree(incstate);
+}
+
/*
* parse the manifest in pieces.
*
diff --git a/src/include/common/parse_manifest.h b/src/include/common/parse_manifest.h
index 3aa594fcac..0d04239c38 100644
--- a/src/include/common/parse_manifest.h
+++ b/src/include/common/parse_manifest.h
@@ -53,5 +53,6 @@ extern JsonManifestParseIncrementalState *json_parse_manifest_incremental_init(J
extern void json_parse_manifest_incremental_chunk(
JsonManifestParseIncrementalState *incstate, char *chunk, int size,
bool is_last);
+extern void json_parse_manifest_incremental_shutdown(JsonManifestParseIncrementalState *incstate);
#endif
