Обсуждение: Avoid incomplete copy string (src/backend/access/transam/xlog.c)

Hi.

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.

So, I think this can result in errors,

like in the function *build_backup_content* (src/backend/access/transam/xlogbackup.c)

Where *appendStringInfo* expects a string with null-termination.

appendStringInfo(result, "LABEL: %s\n", state->name);

To fix, copy strlen size plus one byte, to include the null-termination.

On Sun, 23 Jun 2024 at 20:51 Ranier Vilela <ranier.vf@gmail.com> wrote:

Hi.

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.

So, I think this can result in errors,

like in the function *build_backup_content* (src/backend/access/transam/xlogbackup.c)

Where *appendStringInfo* expects a string with null-termination.

appendStringInfo(result, "LABEL: %s\n", state->name);

To fix, copy strlen size plus one byte, to include the null-termination.

Doesn’t “sizeof” solve the problem? It take in account the null-termination character.

Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier <michael@paquier.xyz> escreveu:

On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote:
> It's not critical code, so I think it's ok to use strlen, even because the
> result of strlen will already be available using modern compilers.
>
> So, I think it's ok to use memcpy with strlen + 1.

It seems to me that there is a pretty good argument to just use
strlcpy() for the same reason as the one you cite: this is not a
performance-critical code, and that's just safer.

Yeah, I'm fine with strlcpy. I'm not against it.

Em dom., 23 de jun. de 2024 às 22:05, Ranier Vilela <ranier.vf@gmail.com> escreveu:

Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier <michael@paquier.xyz> escreveu:

On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote:
> It's not critical code, so I think it's ok to use strlen, even because the
> result of strlen will already be available using modern compilers.
>
> So, I think it's ok to use memcpy with strlen + 1.

It seems to me that there is a pretty good argument to just use
strlcpy() for the same reason as the one you cite: this is not a
performance-critical code, and that's just safer.

Yeah, I'm fine with strlcpy. I'm not against it.

Perhaps, like the v2?

Either v1 or v2, to me, looks good.

best regards,

Ranier Vilela

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?

On Sun, 23 Jun 2024 22:34:03 -0300
Ranier Vilela <ranier.vf@gmail.com> wrote:

> Em dom., 23 de jun. de 2024 às 22:14, Ranier Vilela <ranier.vf@gmail.com>
> escreveu:
>
> > Em dom., 23 de jun. de 2024 às 22:05, Ranier Vilela <ranier.vf@gmail.com>
> > escreveu:
> >
> >> Em dom., 23 de jun. de 2024 às 21:54, Michael Paquier <
> >> michael@paquier.xyz> escreveu:
> >>
> >>> On Sun, Jun 23, 2024 at 09:34:45PM -0300, Ranier Vilela wrote:
> >>> > It's not critical code, so I think it's ok to use strlen, even because
> >>> the
> >>> > result of strlen will already be available using modern compilers.
> >>> >
> >>> > So, I think it's ok to use memcpy with strlen + 1.
> >>>
> >>> It seems to me that there is a pretty good argument to just use
> >>> strlcpy() for the same reason as the one you cite: this is not a
> >>> performance-critical code, and that's just safer.
> >>>
> >> Yeah, I'm fine with strlcpy. I'm not against it.
> >>
> > Perhaps, like the v2?
> >
> > Either v1 or v2, to me, looks good.
> >
> Thinking about, does not make sense the field size MAXPGPATH + 1.
> all other similar fields are just MAXPGPATH.
>
> If we copy MAXPGPATH + 1, it will also be wrong.
> So it is necessary to adjust logbackup.h as well.

I am not sure whether we need to change the size of the field,
but if change it, I wonder it is better to modify the following
message from MAXPGPATH to MAXPGPATH -1.

                                 errmsg("backup label too long (max %d bytes)",
                                                MAXPGPATH)));

>
> So, I think that v3 is ok to fix.
>
> best regards,
> Ranier Vilela
>
> >
> > best regards,
> > Ranier Vilela
> >
> >>


--
Yugo NAGATA <nagata@sraoss.co.jp>

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.

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.