Thread: pgsql: Add missing string terminator
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(+)
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
> 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
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
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
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.
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
> 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