Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Remove useless code in InjectionPointAttach()
- 1d325ad99cb2 19 (unreleased) landed
-
[PATCH]Remove the redundant assignment
Feilong Meng <feelingmeng@foxmail.com> — 2025-12-16T06:18:05Z
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
-
Re: [PATCH]Remove the redundant assignment
Chao Li <li.evan.chao@gmail.com> — 2025-12-16T07:08:25Z
> 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/
-
Re: [PATCH]Remove the redundant assignment
Michael Paquier <michael@paquier.xyz> — 2025-12-16T07:55:28Z
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
-
Re: [PATCH]Remove the redundant assignment
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2025-12-16T11:16:47Z
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
-
Re: [PATCH]Remove the redundant assignment
Heikki Linnakangas <hlinnaka@iki.fi> — 2025-12-16T14:02:53Z
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 -
Re: [PATCH]Remove the redundant assignment
Michael Paquier <michael@paquier.xyz> — 2025-12-17T00:06:35Z
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
-
Re: [PATCH]Remove the redundant assignment
Michael Paquier <michael@paquier.xyz> — 2025-12-17T00:08:02Z
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