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

Em qui., 27 de jun. de 2024 às 01:01, Yugo NAGATA <nagata@sraoss.co.jp> escreveu:

On Mon, 24 Jun 2024 08:25:36 -0300
Ranier Vilela <ranier.vf@gmail.com> wrote:

> Em dom., 23 de jun. de 2024 às 23:56, Richard Guo <guofenglinux@gmail.com>
> escreveu:
>
> > On Mon, Jun 24, 2024 at 7:51 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> > > In src/include/access/xlogbackup.h, the field *name*
> > > has one byte extra to store null-termination.
> > >
> > > But, in the function *do_pg_backup_start*,
> > > I think that is a mistake in the line (8736):
> > >
> > > memcpy(state->name, backupidstr, strlen(backupidstr));
> > >
> > > memcpy with strlen does not copy the whole string.
> > > strlen returns the exact length of the string, without
> > > the null-termination.
> >
> > I noticed that the two callers of do_pg_backup_start both allocate
> > BackupState with palloc0.  Can we rely on this to ensure that the
> > BackupState.name is initialized with null-termination?
> >
> I do not think so.
> It seems to me the best solution is to use Michael's suggestion, strlcpy +
> sizeof.
>
> Currently we have this:
> memcpy(state->name, "longlongpathexample1",
> strlen("longlongpathexample1"));
> printf("%s\n", state->name);
> longlongpathexample1
>
> Next random call:
> memcpy(state->name, "longpathexample2", strlen("longpathexample2"));
> printf("%s\n", state->name);
> longpathexample2ple1

In the current uses, BackupState is freed (by pfree or MemoryContextDelete)
after each use of BackupState, so the memory space is not reused as your
scenario above, and there would not harms even if the null-termination is omitted.

However, I wonder it is better to use strlcpy without assuming such the good
manner of callers.

Thanks for your inputs.

strlcpy is used across all the sources, so this style is better and safe.

Here v4, attached, with MAXPGPATH -1, according to your suggestion.