Horton

Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

Ranier Vilela <ranier.vf@gmail.com>

View thread
From: Ranier Vilela <ranier.vf@gmail.com>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Yugo NAGATA <nagata@sraoss.co.jp>, Richard Guo <guofenglinux@gmail.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2024-07-01T17:35:49Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use safe string copy routine

Attachments

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