Use NameData and namestrcpy for names

[mkindahl]

Using strlcpy to copy variables holding PostgreSQL names can cause issues since names are fixed-size types of length 64. This means that any data that follows the initial null-terminated string will also be part of the data.

Instead of using const char* for PostgreSQL names, use NameData type for PostgreSQL names and use namestrcpy to copy them rather than strlcpy, which is a safe alternative since it will write null characters to the entire buffer.

[codecov]

Codecov Report

Merging #5336 (7cd929f) into main (0963609) will decrease coverage by 0.05%.
The diff coverage is 90.90%.

❗ Current head 7cd929f differs from pull request most recent head 0adf874. Consider uploading reports for the commit 0adf874 to get more accurate results

@@            Coverage Diff             @@
##             main    #5336      +/-   ##
==========================================
- Coverage   90.71%   90.66%   -0.05%     
==========================================
  Files         225      225              
  Lines       52014    51916      -98     
==========================================
- Hits        47183    47072     -111     
- Misses       4831     4844      +13     

Impacted Files	Coverage Δ
src/ts_catalog/catalog.h	100.00% <ø> (ø)
src/partitioning.c	89.08% <90.00%> (+0.25%)	⬆️
src/ts_catalog/catalog.c	85.91% <100.00%> (ø)
src/bgw/scheduler.c	84.19% <0.00%> (-4.30%)	⬇️
src/loader/bgw_launcher.c	89.51% <0.00%> (-2.55%)	⬇️
src/loader/bgw_message_queue.c	86.36% <0.00%> (-2.28%)	⬇️
src/import/planner.c	63.78% <0.00%> (-1.38%)	⬇️
tsl/src/debug.c	50.00% <0.00%> (-0.32%)	⬇️
tsl/src/reorder.c	85.49% <0.00%> (-0.22%)	⬇️
tsl/src/nodes/decompress_chunk/decompress_chunk.c	94.89% <0.00%> (-0.20%)	⬇️
... and 16 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

@jnidzwetzki, @nikkhils: please review this pull request.

Powered by pull-review

[sotirissl]

In the commit message the namedatacpy function is mentioned but in the code the namestrcpy function is been used.

[jnidzwetzki]

@mkindahl We have some further places in the code where NAMEDATALEN char arrays / strlcpy with NAMEDATALEN is used (e.g., here). I used the following coccinelle script to detect them. Could we change these places as well and add the coccinelle script to the repository to prevent the further usage of these arrays?

jan@debian11-work:~/timescaledb/coccinelle$ cat namedata.cocci 
@rule_var_decl@
symbol NAMEDATALEN;
identifier I;
@@

- char I[NAMEDATALEN];
+ /* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+ NameData I;

@rule_var_decl_struct@
symbol NAMEDATALEN;
identifier I1, I2;
@@
struct I1
{
  ...
- char I2[NAMEDATALEN];
+ /* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+ NameData I2;
  ...
}

@rule_strlcpy@
expression E1, E2;
symbol NAMEDATALEN;
@@
- strlcpy(E1, E2, NAMEDATALEN);
+ /* You are using strlcpy with NAMEDATALEN, please consider using namestrcpy instead. */
+ namestrcpy(&E1, E2);

jan@debian11-work:~/timescaledb/coccinelle$ find .. \( -name '*.h' -or -name '*.c' \) -exec spatch --very-quiet --sp-file namedata.cocci {} +

--- ../tsl/src/continuous_aggs/create.c
+++ /tmp/cocci-output-2962036-a00474-create.c
@@ -1867,7 +1867,8 @@ mattablecolumninfo_addentry(MatTableColu
 							bool finalized, bool *skip_adding)
 {
 	int matcolno = list_length(out->matcollist) + 1;
-	char colbuf[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData colbuf;
 	char *colname;
 	TargetEntry *part_te = NULL;
 	ColumnDef *col;
@@ -2618,7 +2619,8 @@ cagg_create(const CreateTableAsStmt *cre
 			CAggTimebucketInfo *origquery_ht, WithClauseResult *with_clause_options)
 {
 	ObjectAddress mataddress;
-	char relnamebuf[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData relnamebuf;
 	MatTableColumnInfo mattblinfo;
 	FinalizeQueryInfo finalqinfo;
 	CatalogSecurityContext sec_ctx;
--- ../tsl/src/compression/create.c
+++ /tmp/cocci-output-2962036-f7d0cf-create.c
@@ -562,7 +562,8 @@ static int32
 create_compression_table(Oid owner, CompressColInfo *compress_cols)
 {
 	ObjectAddress tbladdress;
-	char relnamebuf[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData relnamebuf;
 	CatalogSecurityContext sec_ctx;
 	Datum toast_options;
 	static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
--- ../tsl/src/reorder.c
+++ /tmp/cocci-output-2962036-04d563-reorder.c
@@ -1056,7 +1056,8 @@ finish_heap_swaps(Oid OIDOldHeap, Oid OI
 		if (OidIsValid(newrel->rd_rel->reltoastrelid))
 		{
 			Oid toastidx;
-			char NewToastName[NAMEDATALEN];
+			/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+			NameData NewToastName;
 
 			/* Get the associated valid index to be renamed */
 			toastidx = toast_get_valid_index(newrel->rd_rel->reltoastrelid, AccessShareLock);
--- ../src/bgw/worker.h
+++ /tmp/cocci-output-2962036-27409f-worker.h
@@ -40,7 +40,8 @@ typedef struct BgwParams
 	int32 ttl;
 
 	/** Name of function to call when starting the background worker. */
-	char bgw_main[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData bgw_main;
 } BgwParams;
 
 /**
--- ../src/bgw/job.c
+++ /tmp/cocci-output-2962036-2906e2-job.c
@@ -1339,7 +1339,8 @@ ts_bgw_job_insert_relation(Name applicat
 	CatalogSecurityContext sec_ctx;
 	bool nulls[Natts_bgw_job] = { false };
 	int32 job_id;
-	char app_name[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData app_name;
 
 	rel = table_open(catalog_get_table_id(catalog, BGW_JOB), RowExclusiveLock);
 	desc = RelationGetDescr(rel);
--- ../src/partitioning.h
+++ /tmp/cocci-output-2962036-5c91dc-partitioning.h
@@ -25,8 +25,10 @@
 
 typedef struct PartitioningFunc
 {
-	char schema[NAMEDATALEN];
-	char name[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData schema;
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData name;
 	Oid rettype;
 
 	/*
@@ -38,7 +40,8 @@ typedef struct PartitioningFunc
 
 typedef struct PartitioningInfo
 {
-	char column[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData column;
 	AttrNumber column_attnum;
 	DimensionType dimtype;
 	PartitioningFunc partfunc;
--- ../src/ts_catalog/catalog.h
+++ /tmp/cocci-output-2962036-5f35ad-catalog.h
@@ -1378,7 +1378,8 @@ typedef struct CatalogTableInfo
 
 typedef struct CatalogDatabaseInfo
 {
-	char database_name[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData database_name;
 	Oid database_id;
 	Oid schema_id;
 	Oid owner_uid;
--- ../src/ts_catalog/catalog.c
+++ /tmp/cocci-output-2962036-f6c548-catalog.c
@@ -381,7 +381,8 @@ static void
 catalog_database_info_init(CatalogDatabaseInfo *info)
 {
 	info->database_id = MyDatabaseId;
-	strlcpy(info->database_name, get_database_name(MyDatabaseId), NAMEDATALEN);
+	/* You are using strlcpy with NAMEDATALEN, please consider using namestrcpy instead. */
+	namestrcpy(&info->database_name, get_database_name(MyDatabaseId));
 	info->schema_id = get_namespace_oid(CATALOG_SCHEMA_NAME, false);
 	info->owner_uid = catalog_owner();
 
--- ../src/ts_catalog/metadata.c
+++ /tmp/cocci-output-2962036-f88e00-metadata.c
@@ -141,7 +141,8 @@ ts_metadata_insert(const char *metadata_
 	bool isnull = false;
 	Catalog *catalog = ts_catalog_get();
 	Relation rel;
-	char key_data[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData key_data;
 
 	rel = table_open(catalog_get_table_id(catalog, METADATA), ShareRowExclusiveLock);
 
@@ -157,7 +158,8 @@ ts_metadata_insert(const char *metadata_
 
 	/* We have to copy the key here because heap_form_tuple will copy NAMEDATALEN
 	 * into the tuple instead of checking length. */
-	strlcpy(key_data, metadata_key, NAMEDATALEN);
+	/* You are using strlcpy with NAMEDATALEN, please consider using namestrcpy instead. */
+	namestrcpy(&key_data, metadata_key);
 
 	/* Insert into the catalog table for persistence */
 	values[AttrNumberGetAttrOffset(Anum_metadata_key)] = CStringGetDatum(key_data);
--- ../src/partitioning.c
+++ /tmp/cocci-output-2962036-08e58f-partitioning.c
@@ -184,8 +184,10 @@ ts_partitioning_info_create(const char *
 				 errmsg("partitioning function information cannot be null")));
 
 	pinfo = palloc0(sizeof(PartitioningInfo));
-	strlcpy(pinfo->partfunc.name, partfunc, NAMEDATALEN);
-	strlcpy(pinfo->column, partcol, NAMEDATALEN);
+	/* You are using strlcpy with NAMEDATALEN, please consider using namestrcpy instead. */
+	namestrcpy(&pinfo->partfunc.name, partfunc);
+	/* You are using strlcpy with NAMEDATALEN, please consider using namestrcpy instead. */
+	namestrcpy(&pinfo->column, partcol);
 	pinfo->column_attnum = get_attnum(relid, pinfo->column);
 	pinfo->dimtype = dimtype;
 
@@ -193,7 +195,8 @@ ts_partitioning_info_create(const char *
 	if (pinfo->column_attnum == InvalidAttrNumber)
 		return NULL;
 
-	strlcpy(pinfo->partfunc.schema, schema, NAMEDATALEN);
+	/* You are using strlcpy with NAMEDATALEN, please consider using namestrcpy instead. */
+	namestrcpy(&pinfo->partfunc.schema, schema);
 
 	/* Lookup the type cache entry to access the hash function for the type */
 	columntype = get_atttype(relid, pinfo->column_attnum);
--- ../src/chunk_constraint.c
+++ /tmp/cocci-output-2962036-412d3c-chunk_constraint.c
@@ -91,7 +91,8 @@ chunk_constraint_dimension_choose_name(N
 static void
 chunk_constraint_choose_name(Name dst, const char *hypertable_constraint_name, int32 chunk_id)
 {
-	char constrname[NAMEDATALEN];
+	/* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+	NameData constrname;
 	CatalogSecurityContext sec_ctx;
 
 	Assert(hypertable_constraint_name != NULL);

[mkindahl]

@jnidzwetzki Absolutely. I used LSP to try to find the references, but it did not seem to catch them all. Will look into the list. Note that there is one case already handled by #5317 so I will skip that one.

[kgyrtkirk]

I was looking at NAMEDATALEN occurances to see what coccinelle could catch - and something caught my eye; there are comparisions against NAMEDATALEN to ensure that the identifier fits into one; but there are a few places where > is used instead >=.

I'm was using git grep NAMEDATALEN|grep '[<>]'; but all of them can be matched with the coccinelle rule:

@rule_var_decl@
symbol NAMEDATALEN;
expression E;
@@

- E > NAMEDATALEN
+ E >= NAMEDATALEN /* are you checking that the identifier would fit? */

[jnidzwetzki]

@kgyrtkirk This rule also looks great. If it does not produce any false positives, I am in favor of including it 👍.

[mkindahl]

@rule_var_decl@
symbol NAMEDATALEN;
identifier I;
@@

- char I[NAMEDATALEN];
+ /* You are declaring a char of length NAMEDATALEN, please consider using NameData instead. */
+ NameData I;

Hmm... I am not sure about this rule. It is quite common in PostgreSQL code to use this pattern and then use namestrcpy to copy into the "real" buffer and the second parameter of namestrcpy is a const char* so a buffer is expected.

[jnidzwetzki]

You are right, these rules seem to be too generic. Maybe we should remove the first two rules and only use the rule for strlcpy.

[mkindahl]

You are right, these rules seem to be too generic. Maybe we should remove the first two rules and only use the rule for strlcpy.

It seems that leaves only the one in #5317 unpatched.

mats@fury:~/work/timescale/timescaledb+bugs$ cat namedata.cocci
@rule_var_decl_struct@
symbol NAMEDATALEN;
identifier I1, I2;
@@
struct I1
{
  ...
- char I2[NAMEDATALEN];
+ NameData I2;
  ...
}

@rule_strlcpy@
expression E1, E2;
symbol NAMEDATALEN;
@@
- strlcpy(E1, E2, NAMEDATALEN);
+ namestrcpy(E1, E2);
mats@fury:~/work/timescale/timescaledb+bugs$ spatch --very-quiet --sp-file namedata.cocci --dir .
diff -u -p a/home/mats/work/timescale/timescaledb+bugs/src/ts_catalog/metadata.c b/home/mats/work/timescale/timescaledb+bugs/src/ts_catalog/metadata.c
--- a/home/mats/work/timescale/timescaledb+bugs/src/ts_catalog/metadata.c
+++ b/home/mats/work/timescale/timescaledb+bugs/src/ts_catalog/metadata.c
@@ -157,7 +157,7 @@ ts_metadata_insert(const char *metadata_
 
        /* We have to copy the key here because heap_form_tuple will copy NAMEDATALEN
         * into the tuple instead of checking length. */
-       strlcpy(key_data, metadata_key, NAMEDATALEN);
+       namestrcpy(key_data, metadata_key);
 
        /* Insert into the catalog table for persistence */
        values[AttrNumberGetAttrOffset(Anum_metadata_key)] = CStringGetDatum(key_data);

[jnidzwetzki]

Looks good to me. What do you think? Should we add the Coccinelle rule in this PR or in a follow-up PR?

[mkindahl]

Looks good to me. What do you think? Should we add the Coccinelle rule in this PR or in a follow-up PR?

Since it will generate diffs until #5317 is merged, should probably wait until that.