Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c) - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Date
Msg-id CAEudQAoqnRbz27iLv7sHziw2t0zy032oUpzWFxAE=P7pdOKaeg@mail.gmail.com
Whole thread Raw
In response to Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)  (Daniel Gustafsson <daniel@yesql.se>)
Responses Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
List pgsql-hackers
Em seg., 1 de jul. de 2024 às 06:20, Daniel Gustafsson <daniel@yesql.se> escreveu:
> On 27 Jun 2024, at 13:50, Ranier Vilela <ranier.vf@gmail.com> wrote:

> Now with file patch really attached.

-       if (strlen(backupidstr) > MAXPGPATH)
+       if (strlcpy(state->name, backupidstr, sizeof(state->name)) >= sizeof(state->name))
                ereport(ERROR,

Stylistic nit perhaps, I would keep the strlen check here and just replace the
memcpy with strlcpy.  Using strlen in the error message check makes the code
more readable.
This is not performance-critical code, so I see no problem using strlen, for the sake of readability.



-       char            name[MAXPGPATH + 1];
+       char            name[MAXPGPATH];/* backup label name */

With the introduced use of strlcpy, why do we need to change this field?
The part about being the only reference in the entire code that uses MAXPGPATH + 1.
MAXPGPATH is defined as 1024, so MAXPGPATH +1 is 1025.
I think this hurts the calculation of the array index,
preventing power two optimization.

Another argument is that all other paths have a 1023 size limit,
I don't see why the backup label would have to be different.

New version patch attached.

best regards,
Ranier Vilela
Attachment

pgsql-hackers by date:

Previous
From: Andres Freund
Date:
Subject: Re: LogwrtResult contended spinlock
Next
From: Ranier Vilela
Date:
Subject: Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)