Shouldn't execParallel.c null-terminate query_string in the parallel DSM?

Started by Thomas Munroabout 8 years ago3 messages
#1Thomas Munro
thomas.munro@enterprisedb.com
1 attachment(s)

Hi hackers,

I just saw some trailing garbage in my log file emanating from a
parallel worker when my query happened to be a BUFFERALIGNed length
(in this case 64 characters). Did commit 4c728f382970 forget to make
sure the null terminator is copied? See attached proposed fix.

--
Thomas Munro
http://www.enterprisedb.com

Attachments:

null-terminate-query-string.patchapplication/octet-stream; name=null-terminate-query-string.patchDownload
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index 02b5aa517b..604f4f5b61 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -597,7 +597,7 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate,
 
 	/* Estimate space for query text. */
 	query_len = strlen(estate->es_sourceText);
-	shm_toc_estimate_chunk(&pcxt->estimator, query_len);
+	shm_toc_estimate_chunk(&pcxt->estimator, query_len + 1);
 	shm_toc_estimate_keys(&pcxt->estimator, 1);
 
 	/* Estimate space for serialized PlannedStmt. */
@@ -672,8 +672,8 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate,
 	shm_toc_insert(pcxt->toc, PARALLEL_KEY_EXECUTOR_FIXED, fpes);
 
 	/* Store query string */
-	query_string = shm_toc_allocate(pcxt->toc, query_len);
-	memcpy(query_string, estate->es_sourceText, query_len);
+	query_string = shm_toc_allocate(pcxt->toc, query_len + 1);
+	memcpy(query_string, estate->es_sourceText, query_len + 1);
 	shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, query_string);
 
 	/* Store serialized PlannedStmt. */
#2Rafia Sabih
rafia.sabih@enterprisedb.com
In reply to: Thomas Munro (#1)
Re: Shouldn't execParallel.c null-terminate query_string in the parallel DSM?

On Wed, Dec 20, 2017 at 7:58 AM, Thomas Munro <thomas.munro@enterprisedb.com

wrote:

Hi hackers,

I just saw some trailing garbage in my log file emanating from a
parallel worker when my query happened to be a BUFFERALIGNed length
(in this case 64 characters). Did commit 4c728f382970 forget to make
sure the null terminator is copied? See attached proposed fix.

Yes, I missed that.

Your patch looks good to me, thanks.

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/

#3Robert Haas
robertmhaas@gmail.com
In reply to: Rafia Sabih (#2)
Re: Shouldn't execParallel.c null-terminate query_string in the parallel DSM?

On Wed, Dec 20, 2017 at 2:03 AM, Rafia Sabih
<rafia.sabih@enterprisedb.com> wrote:

On Wed, Dec 20, 2017 at 7:58 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:

I just saw some trailing garbage in my log file emanating from a
parallel worker when my query happened to be a BUFFERALIGNed length
(in this case 64 characters). Did commit 4c728f382970 forget to make
sure the null terminator is copied? See attached proposed fix.

Yes, I missed that.
Your patch looks good to me, thanks.

Sigh. I missed that too. Committed and back-patched to v10.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company