[PATCH]Remove the redundant assignment
Hi,Hackers,
I found that in the InjectionPointAttach function within the src/backend/utils/misc/injection_point.c file, the variable is manually assigned a '\0' at the end, even though strlcpy already guarantees that the destination buffer will be null-terminated and will not overflow.
The code modification is as follows:
```
/* Save the entry */
strlcpy(entry->name, name, sizeof(entry->name));
entry->name[INJ_NAME_MAXLEN - 1] = '\0'; <== Delete this line
strlcpy(entry->library, library, sizeof(entry->library));
entry->library[INJ_LIB_MAXLEN - 1] = '\0'; <== Delete this line
strlcpy(entry->function, function, sizeof(entry->function));
entry->function[INJ_FUNC_MAXLEN - 1] = '\0'; <== Delete this line
```
And in the injection_point_cache_add function within the same file, strlcpy(entry->name, name, sizeof(entry->name)); does not perform redundant assignment.
I have tested the change, and "make check" passed.
--------------
Best regards,
Feilong Meng
Attachments:
0001-Remove-the-redundant-assignment-in-InjectionPointAtt.patchapplication/octet-stream; name=0001-Remove-the-redundant-assignment-in-InjectionPointAtt.patchDownload
From 74d67428d0a5d5993ec62ec0641bfa9101dd8ed8 Mon Sep 17 00:00:00 2001
From: Feilong Meng <feelingmeng@foxmail.com>
Date: Tue, 16 Dec 2025 13:50:12 +0800
Subject: [PATCH] Remove the redundant assignment in InjectionPointAttach
Remove the redundant assignment in the InjectionPointAttach function within the injection_point.c file.
Author: Feilong Meng <feelingmeng@foxmail.com>
---
src/backend/utils/misc/injection_point.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 54a9fe8..4945da4 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -331,11 +331,8 @@ InjectionPointAttach(const char *name,
/* Save the entry */
strlcpy(entry->name, name, sizeof(entry->name));
- entry->name[INJ_NAME_MAXLEN - 1] = '\0';
strlcpy(entry->library, library, sizeof(entry->library));
- entry->library[INJ_LIB_MAXLEN - 1] = '\0';
strlcpy(entry->function, function, sizeof(entry->function));
- entry->function[INJ_FUNC_MAXLEN - 1] = '\0';
if (private_data != NULL)
memcpy(entry->private_data, private_data, private_data_size);
--
1.8.3.1
On Dec 16, 2025, at 14:18, Feilong Meng <feelingmeng@foxmail.com> wrote:
Hi,Hackers,
I found that in the InjectionPointAttach function within the src/backend/utils/misc/injection_point.c file, the variable is manually assigned a '\0' at the end, even though strlcpy already guarantees that the destination buffer will be null-terminated and will not overflow.
The code modification is as follows:
```
/* Save the entry */
strlcpy(entry->name, name, sizeof(entry->name));
entry->name[INJ_NAME_MAXLEN - 1] = '\0'; <== Delete this line
strlcpy(entry->library, library, sizeof(entry->library));
entry->library[INJ_LIB_MAXLEN - 1] = '\0'; <== Delete this line
strlcpy(entry->function, function, sizeof(entry->function));
entry->function[INJ_FUNC_MAXLEN - 1] = '\0'; <== Delete this line
```And in the injection_point_cache_add function within the same file, strlcpy(entry->name, name, sizeof(entry->name)); does not perform redundant assignment.
I have tested the change, and "make check" passed.
Indeed, explicitly adding a trailing '\0' is redundant here, since strlcpy() already guarantees NULL termination.
I also noticed that in the same file, another code path does not perform this extra assignment:
```
Assert(!found);
strlcpy(entry->name, name, sizeof(entry->name));
entry->slot_idx = slot_idx;
entry->generation = generation;
entry->callback = callback;
memcpy(entry->private_data, private_data, INJ_PRIVATE_MAXLEN);
```
Given that, I agree we should remove the redundant assignments to keep the code clearer and consistent.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Tue, Dec 16, 2025 at 03:08:25PM +0800, Chao Li wrote:
Given that, I agree we should remove the redundant assignments to
keep the code clearer and consistent.
Yeah, these could be removed. I am wondering why 0eb23285a257 did not
bother, but that's no big deal one way or another, just less code at
the end of the day.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Tue, Dec 16, 2025 at 03:08:25PM +0800, Chao Li wrote:
Given that, I agree we should remove the redundant assignments to
keep the code clearer and consistent.Yeah, these could be removed. I am wondering why 0eb23285a257 did not
bother, but that's no big deal one way or another, just less code at
the end of the day.
A quick grep reveals a bunch of strncpy() calls followed by a '\0'
assignment that could be replaced with strlcpy():
$ rg -A1 strncpy|rg -B1 "= '\\\\0';"
src/interfaces/libpq/fe-secure-openssl.c: strncpy(buf, conn->sslpassword, size);
src/interfaces/libpq/fe-secure-openssl.c- buf[size - 1] = '\0';
--
src/bin/pgbench/pgbench.c: strncpy(*script, option, namelen);
src/bin/pgbench/pgbench.c- (*script)[namelen] = '\0';
--
doc/src/sgml/ecpg.sgml: strncpy(name_buf, v.sqlname.data, v.sqlname.length);
doc/src/sgml/ecpg.sgml- name_buf[v.sqlname.length] = '\0';
--
doc/src/sgml/ecpg.sgml: strncpy(name_buf, v.sqlname.data, v.sqlname.length);
doc/src/sgml/ecpg.sgml- name_buf[v.sqlname.length] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(newcopy, (char *) var->value, slen);
src/interfaces/ecpg/ecpglib/execute.c- newcopy[slen] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(mallocedval, (char *) var->value, slen);
src/interfaces/ecpg/ecpglib/execute.c- mallocedval[slen] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(newcopy, variable->arr, variable->len);
src/interfaces/ecpg/ecpglib/execute.c- newcopy[variable->len] = '\0';
--
src/backend/utils/adt/name.c: strncpy(NameStr(*name), str, NAMEDATALEN);
src/backend/utils/adt/name.c- NameStr(*name)[NAMEDATALEN - 1] = '\0';
- ilmari
On 16/12/2025 13:16, Dagfinn Ilmari Mannsåker wrote:
A quick grep reveals a bunch of strncpy() calls followed by a '\0'
assignment that could be replaced with strlcpy():$ rg -A1 strncpy|rg -B1 "= '\\\\0';"
src/interfaces/libpq/fe-secure-openssl.c: strncpy(buf, conn->sslpassword, size);
src/interfaces/libpq/fe-secure-openssl.c- buf[size - 1] = '\0';
I'm not sure what exactly this code does, but it seems prudent to zero
the unused bytes since we're dealing with a password.
--
src/bin/pgbench/pgbench.c: strncpy(*script, option, namelen);
src/bin/pgbench/pgbench.c- (*script)[namelen] = '\0';
Yeah, this one could use strlcpy(). Or memcpy(). Or pstrndup().
--
doc/src/sgml/ecpg.sgml: strncpy(name_buf, v.sqlname.data, v.sqlname.length);
doc/src/sgml/ecpg.sgml- name_buf[v.sqlname.length] = '\0';
--
doc/src/sgml/ecpg.sgml: strncpy(name_buf, v.sqlname.data, v.sqlname.length);
doc/src/sgml/ecpg.sgml- name_buf[v.sqlname.length] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(newcopy, (char *) var->value, slen);
src/interfaces/ecpg/ecpglib/execute.c- newcopy[slen] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(mallocedval, (char *) var->value, slen);
src/interfaces/ecpg/ecpglib/execute.c- mallocedval[slen] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(newcopy, variable->arr, variable->len);
src/interfaces/ecpg/ecpglib/execute.c- newcopy[variable->len] = '\0';
I don't know if these depend on the zero-padding...
--
src/backend/utils/adt/name.c: strncpy(NameStr(*name), str, NAMEDATALEN);
src/backend/utils/adt/name.c- NameStr(*name)[NAMEDATALEN - 1] = '\0';
This one *does* require the zero-padding, there's a comment that says so:
void
namestrcpy(Name name, const char *str)
{
/* NB: We need to zero-pad the destination. */
strncpy(NameStr(*name), str, NAMEDATALEN);
NameStr(*name)[NAMEDATALEN - 1] = '\0';
}
- Heikki
On Tue, Dec 16, 2025 at 04:02:53PM +0200, Heikki Linnakangas wrote:
doc/src/sgml/ecpg.sgml: strncpy(name_buf, v.sqlname.data, v.sqlname.length);
doc/src/sgml/ecpg.sgml- name_buf[v.sqlname.length] = '\0';
--
doc/src/sgml/ecpg.sgml: strncpy(name_buf, v.sqlname.data, v.sqlname.length);
doc/src/sgml/ecpg.sgml- name_buf[v.sqlname.length] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(newcopy, (char *) var->value, slen);
src/interfaces/ecpg/ecpglib/execute.c- newcopy[slen] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(mallocedval, (char *) var->value, slen);
src/interfaces/ecpg/ecpglib/execute.c- mallocedval[slen] = '\0';
--
src/interfaces/ecpg/ecpglib/execute.c: strncpy(newcopy, variable->arr, variable->len);
src/interfaces/ecpg/ecpglib/execute.c- newcopy[variable->len] = '\0';I don't know if these depend on the zero-padding...
Good question. This code has never been changed since its
introduction in a4f25b6a9c2d...
But anyway, looking more closely I think that we should be OK with
just switching to strlcpy() as long as we do the call with a "len + 1"
and not "len" to account for the zero termination based on the
allocations done just before copying the values.
--
Michael
On Tue, Dec 16, 2025 at 04:55:28PM +0900, Michael Paquier wrote:
Yeah, these could be removed. I am wondering why 0eb23285a257 did not
bother, but that's no big deal one way or another, just less code at
the end of the day.
And applied a patch to remove this code from InjectionPointAttach().
Thanks.
--
Michael