pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Add missing string terminator
8+ messages / 4 participants
[nested] [flat]
* pgsql: Add missing string terminator
@ 2025-04-30 09:36 Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Gustafsson @ 2025-04-30 09:36 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Add missing string terminator
When copying the string strncpy won't add nul termination since
the string length is equal to the length specified. Explicitly
set a nul terminator after copying to properly terminate. Found
via post-commit review.
Author: Rahila Syed <rahilasyed90@gmail.com>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAH2L28vt16C9xTuK+K7QZvtA3kCNWXOEiT=gEekUw3Xxp9LVQw@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/d2a1ed1727a8ef45eab1a8ddb3d375c1ce839aac
Modified Files
--------------
src/backend/utils/mmgr/mcxt.c | 1 +
1 file changed, 1 insertion(+)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
@ 2025-04-30 10:57 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 8+ messages in thread
From: David Rowley @ 2025-04-30 10:57 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-committers@lists.postgresql.org
On Wed, 30 Apr 2025 at 21:36, Daniel Gustafsson
<dgustafsson@postgresql.org> wrote:
> Add missing string terminator
A possible minor niggle. Would memcpy not be a more suitable function
for this? It's just there've been a few efforts in the past to try
and minimise the usage of strncpy(). There should really just be a
small handful of cases where the strange behaviour of that function is
needed. I don't think this is one of those cases.
Maybe something like the following would suit better?
#define REMAINING_TOTALS "Remaining Totals"
num_individual_stats = context_id + 1;
meminfo[max_stats - 1].name = dsa_allocate(MemoryStatsDsaArea,
sizeof(REMAINING_TOTALS));
nameptr = dsa_get_address(MemoryStatsDsaArea, meminfo[max_stats - 1].name);
memcpy(nameptr, REMAINING_TOTALS, sizeof(REMAINING_TOTALS));
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
@ 2025-04-30 11:27 ` Daniel Gustafsson <daniel@yesql.se>
2025-04-30 11:43 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Gustafsson @ 2025-04-30 11:27 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: pgsql-committers@lists.postgresql.org
> On 30 Apr 2025, at 12:57, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Wed, 30 Apr 2025 at 21:36, Daniel Gustafsson
> <dgustafsson@postgresql.org> wrote:
>> Add missing string terminator
>
> A possible minor niggle. Would memcpy not be a more suitable function
> for this?
How about using strlcpy as suggested by Peter?
- strncpy(nameptr, "Remaining Totals", namelen);
- nameptr[namelen] = '\0';
+ strlcpy(nameptr, "Remaining Totals", namelen + 1);
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
@ 2025-04-30 11:43 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:56 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: David Rowley @ 2025-04-30 11:43 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-committers@lists.postgresql.org
On Wed, 30 Apr 2025 at 23:27, Daniel Gustafsson <daniel@yesql.se> wrote:
> How about using strlcpy as suggested by Peter?
>
> - strncpy(nameptr, "Remaining Totals", namelen);
> - nameptr[namelen] = '\0';
> + strlcpy(nameptr, "Remaining Totals", namelen + 1);
Sorry, I only noticed that conversation afterwards. Yeah, strlcpy()
is fine too. memcpy() would make more sense IMO, since the length is
known already. I'm fine with either, however.
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
2025-04-30 11:43 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
@ 2025-04-30 11:56 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 12:44 ` Re: pgsql: Add missing string terminator Peter Eisentraut <peter@eisentraut.org>
0 siblings, 1 reply; 8+ messages in thread
From: David Rowley @ 2025-04-30 11:56 UTC (permalink / raw)
To: Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-committers@lists.postgresql.org
On Wed, 30 Apr 2025 at 23:43, David Rowley <dgrowleyml@gmail.com> wrote:
> memcpy() would make more sense IMO, since the length is
> known already. I'm fine with either, however.
In case you're looking for inspiration on a standard to follow,
commits such as 586dd5d6a did seem to favour memcpy() when the length
was known and only use strlcpy() when it wasn't.
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
2025-04-30 11:43 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:56 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
@ 2025-04-30 12:44 ` Peter Eisentraut <peter@eisentraut.org>
2025-04-30 13:14 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Peter Eisentraut @ 2025-04-30 12:44 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; Daniel Gustafsson <daniel@yesql.se>; +Cc: pgsql-committers@lists.postgresql.org
On 30.04.25 13:56, David Rowley wrote:
> On Wed, 30 Apr 2025 at 23:43, David Rowley <dgrowleyml@gmail.com> wrote:
>> memcpy() would make more sense IMO, since the length is
>> known already. I'm fine with either, however.
>
> In case you're looking for inspiration on a standard to follow,
> commits such as 586dd5d6a did seem to favour memcpy() when the length
> was known and only use strlcpy() when it wasn't.
It looks like the memcpy() uses in that commit are for cases where we
don't want/need the null terminator.
I think it's best in general to use str* for strings and mem* for
not-strings. That's easier to read and also better for static analyzers
etc.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
2025-04-30 11:43 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:56 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 12:44 ` Re: pgsql: Add missing string terminator Peter Eisentraut <peter@eisentraut.org>
@ 2025-04-30 13:14 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 15:33 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
0 siblings, 1 reply; 8+ messages in thread
From: David Rowley @ 2025-04-30 13:14 UTC (permalink / raw)
To: Peter Eisentraut <peter@eisentraut.org>; +Cc: Daniel Gustafsson <daniel@yesql.se>; pgsql-committers@lists.postgresql.org
On Thu, 1 May 2025 at 00:44, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 30.04.25 13:56, David Rowley wrote:
> > In case you're looking for inspiration on a standard to follow,
> > commits such as 586dd5d6a did seem to favour memcpy() when the length
> > was known and only use strlcpy() when it wasn't.
>
> It looks like the memcpy() uses in that commit are for cases where we
> don't want/need the null terminator.
hmm, all the execute.c ones look like they're copying the NUL char to me.
> I think it's best in general to use str* for strings and mem* for
> not-strings. That's easier to read and also better for static analyzers
> etc.
The reason I think memcpy is better is that the NUL only needs to be
found once. memcpy() is much faster than strlcpy() because it can
operate on many bytes at once rather than doing 1 byte at a time.
Performance might not matter for this case, but your reasoning isn't
specific to this case either.
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: pgsql: Add missing string terminator
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Re: pgsql: Add missing string terminator Daniel Gustafsson <daniel@yesql.se>
2025-04-30 11:43 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:56 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
2025-04-30 12:44 ` Re: pgsql: Add missing string terminator Peter Eisentraut <peter@eisentraut.org>
2025-04-30 13:14 ` Re: pgsql: Add missing string terminator David Rowley <dgrowleyml@gmail.com>
@ 2025-04-30 15:33 ` Daniel Gustafsson <daniel@yesql.se>
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Gustafsson @ 2025-04-30 15:33 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Peter Eisentraut <peter@eisentraut.org>; pgsql-committers@lists.postgresql.org
> On 30 Apr 2025, at 15:14, David Rowley <dgrowleyml@gmail.com> wrote:
> On Thu, 1 May 2025 at 00:44, Peter Eisentraut <peter@eisentraut.org> wrote:
>> I think it's best in general to use str* for strings and mem* for
>> not-strings. That's easier to read and also better for static analyzers
>> etc.
>
> The reason I think memcpy is better is that the NUL only needs to be
> found once. memcpy() is much faster than strlcpy() because it can
> operate on many bytes at once rather than doing 1 byte at a time.
I don't disagree, but since similar codepaths in this file use strlcpy now I
will opt for that for consistency (and thus readability) with a TODO item to
look at changing all these for memcpy during v19.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-04-30 15:33 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-30 09:36 pgsql: Add missing string terminator Daniel Gustafsson <dgustafsson@postgresql.org>
2025-04-30 10:57 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:27 ` Daniel Gustafsson <daniel@yesql.se>
2025-04-30 11:43 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 11:56 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 12:44 ` Peter Eisentraut <peter@eisentraut.org>
2025-04-30 13:14 ` David Rowley <dgrowleyml@gmail.com>
2025-04-30 15:33 ` Daniel Gustafsson <daniel@yesql.se>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox