Fix some memory leaks associated with parsing json and manifests

adunstan · adunstan · commit 661ab4e18578 · 2024-04-12T10:32:30.000-04:00
Coverity complained about not freeing some memory associated with
incrementally parsing backup manifests. To fix that, provide and use a new
shutdown function for the JsonManifestParseIncrementalState object, in
line with a suggestion from Tom Lane.

While analysing the problem, I noticed a buglet in freeing memory for
incremental json lexers. To fix that remove a bogus condition on
freeing the memory allocated for them.
diff --git 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
@@ -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
@@ -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
@@ -488,19 +488,18 @@ freeJsonLexContext(JsonLexContext *lex)
 	if (lex->errormsg)
 		destroyStringInfo(lex->errormsg);
 
-	if (lex->flags & JSONLEX_FREE_STRUCT)
+	if (lex->incremental)
 	{
-		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);
+		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)
+		pfree(lex);
 }
 
 /*
diff --git 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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -241,6 +241,9 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib)`
`241`	`241`	`pfree(ib->buf.data);`
`242`	`242`	`ib->buf.data = NULL;`
`243`	`243`
	`244`	`+ /* Done with inc_state, so release that memory too */`
	`245`	`+ json_parse_manifest_incremental_shutdown(ib->inc_state);`
	`246`	`+`
`244`	`247`	`/* Switch back to previous memory context. */`
`245`	`248`	`MemoryContextSwitchTo(oldcontext);`
`246`	`249`	`}`
Original file line number	Diff line number	Diff line change
`@@ -208,6 +208,9 @@ load_backup_manifest(char *backup_directory)`
`208`	`208`	`inc_state, buffer, rc, bytes_left == 0);`
`209`	`209`	`}`
`210`	`210`
	`211`	`+ /* Release the incremental state memory */`
	`212`	`+ json_parse_manifest_incremental_shutdown(inc_state);`
	`213`	`+`
`211`	`214`	`close(fd);`
`212`	`215`	`}`
`213`	`216`
Original file line number	Diff line number	Diff line change
`@@ -484,6 +484,9 @@ parse_manifest_file(char *manifest_path)`
`484`	`484`	`inc_state, buffer, rc, bytes_left == 0);`
`485`	`485`	`}`
`486`	`486`
	`487`	`+ /* Release the incremental state memory */`
	`488`	`+ json_parse_manifest_incremental_shutdown(inc_state);`
	`489`	`+`
`487`	`490`	`close(fd);`
`488`	`491`	`}`
`489`	`492`