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 CAEudQAqkDPpQDOnAg_21w5_Sz8Z7jomPHsP-nqajo_=WoUKCzw@mail.gmail.com
Whole thread Raw
In response to Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)  (Yugo NAGATA <nagata@sraoss.co.jp>)
Responses Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)
List pgsql-hackers
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.

From the linux man page:

" The strlcpy() function copies up to size - 1 characters from the NUL-terminated string src to dst, NUL-terminating the result. "

best regards,
Ranier Vilela

pgsql-hackers by date:

Previous
From: Amit Langote
Date:
Subject: Re: pgsql: Add more SQL/JSON constructor functions
Next
From: Ranier Vilela
Date:
Subject: Re: Avoid incomplete copy string (src/backend/access/transam/xlog.c)