Horton

Thread

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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-23T23:51:26Z

    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.

Trivial patch attached.

best regards,
Ranier Vilela

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

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2024-06-24T00:08:47Z

    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.
Fabrízio de Royes Mello

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

    Michael Paquier <michael@paquier.xyz> — 2024-06-24T00:23:50Z

    On Sun, Jun 23, 2024 at 09:08:47PM -0300, Fabrízio de Royes Mello wrote:
> Doesn’t “sizeof” solve the problem? It take in account the null-termination
> character.

The size of BackupState->name is fixed with MAXPGPATH + 1, so it would
be a better practice to use strlcpy() with sizeof(name) anyway?
--
Michael

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T00:31:51Z

    Em dom., 23 de jun. de 2024 às 21:08, Fabrízio de Royes Mello <
fabriziomello@gmail.com> escreveu:

>
> 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.

sizeof is is preferable when dealing with constants such as:
memcpy(name, "string test1", sizeof( "string test1");

Using sizeof in this case will always copy MAXPGPATH + 1.
Modern compilers will optimize strlen,
copying fewer bytes.

best regards,
Ranier Vilela

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T00:34:45Z

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

> On Sun, Jun 23, 2024 at 09:08:47PM -0300, Fabrízio de Royes Mello wrote:
> > Doesn’t “sizeof” solve the problem? It take in account the
> null-termination
> > character.
>
> The size of BackupState->name is fixed with MAXPGPATH + 1, so it would
> be a better practice to use strlcpy() with sizeof(name) anyway?
>
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.

best regards,
Ranier Vilela

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

    Michael Paquier <michael@paquier.xyz> — 2024-06-24T00:53:52Z

    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.
--
Michael

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T01:05:42Z

    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.

New version, attached.

best regards,
Ranier Vilela

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T01:14:01Z

    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

>

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T01:34:03Z

    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.

So, I think that v3 is ok to fix.

best regards,
Ranier Vilela

>
> best regards,
> Ranier Vilela
>
>>

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

    Richard Guo <guofenglinux@gmail.com> — 2024-06-24T02:56:20Z

    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?

Thanks
Richard

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

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-06-24T03:27:28Z

    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)));

Regards,
Yugo Nagata

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


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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T11:25:36Z

    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

It's not a good idea to use memcpy with strlen.

best regards,
Ranier Vilela

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-24T11:37:26Z

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

> 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)));
>
Or perhaps, is it better to show the too long label?

errmsg("backup label too long (%s)",
                                                backupidstr)));

best regards,
Ranier Vilela

>
> >
> > So, I think that v3 is ok to fix.
> >
> > best regards,
> > Ranier Vilela
> >
> > >
> > > best regards,
> > > Ranier Vilela
> > >
> > >>
>
>
> --
> Yugo NAGATA <nagata@sraoss.co.jp>
>

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

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-06-27T03:17:04Z

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

> Em seg., 24 de jun. de 2024 às 00:27, Yugo NAGATA <nagata@sraoss.co.jp>
> escreveu:
> 
> > 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)));
> >
> Or perhaps, is it better to show the too long label?
> 
> errmsg("backup label too long (%s)",
>                                                 backupidstr)));

I don't think it is better to show a string longer than MAXPGPATH (=1024)
in the error message.

Regards,
Yugo Nagata

> 
> best regards,
> Ranier Vilela
> 
> >
> > >
> > > So, I think that v3 is ok to fix.
> > >
> > > best regards,
> > > Ranier Vilela
> > >
> > > >
> > > > best regards,
> > > > Ranier Vilela
> > > >
> > > >>
> >
> >
> > --
> > Yugo NAGATA <nagata@sraoss.co.jp>
> >


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

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

    Yugo Nagata <nagata@sraoss.co.jp> — 2024-06-27T04:01:08Z

    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.

Regards,
Yugo Nagata

> 
> It's not a good idea to use memcpy with strlen.
> 
> best regards,
> Ranier Vilela


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

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

    Michael Paquier <michael@paquier.xyz> — 2024-06-27T04:04:22Z

    On Thu, Jun 27, 2024 at 12:17:04PM +0900, Yugo NAGATA wrote:
> I don't think it is better to show a string longer than MAXPGPATH (=1024)
> in the error message.

+1.  I am not convinced that this is useful in this context.
--
Michael

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-27T11:48:47Z

    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:
https://linux.die.net/man/3/strlcpy

" 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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-27T11:50:03Z

    Em qui., 27 de jun. de 2024 às 08:48, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

> 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.
>
Now with file patch really attached.

best regards,
Ranier Vilela

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-01T08:52:22Z

    > On 27 Jun 2024, at 06:01, Yugo NAGATA <nagata@sraoss.co.jp> 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:

>>>> 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.

In this case we can, we do that today..

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

..that being said I agree that it seems safer to use strlcpy() here.

--
Daniel Gustafsson

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-01T09:20:19Z

    > 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.


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

With the introduced use of strlcpy, why do we need to change this field?

--
Daniel Gustafsson

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-01T17:35:49Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-01T17:38:20Z

    Em seg., 1 de jul. de 2024 às 14:35, Ranier Vilela <ranier.vf@gmail.com>
escreveu:

> 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.
>
Sorry for v5, I forgot to update the patch and it was an error.

best regards,
Ranier Vilela

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

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-07-01T19:15:48Z

    On 2024-Jul-01, Ranier Vilela wrote:

> > -       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.

The bit I don't understand about this discussion is what will happen
with users that currently have exactly 1024 chars in backup names today.
With this change, we'll be truncating their names to 1023 chars instead.
Why would they feel that such change is welcome?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-01T19:19:59Z

    > On 1 Jul 2024, at 21:15, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2024-Jul-01, Ranier Vilela wrote:

>>> -       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.
> 
> The bit I don't understand about this discussion is what will happen
> with users that currently have exactly 1024 chars in backup names today.
> With this change, we'll be truncating their names to 1023 chars instead.
> Why would they feel that such change is welcome?

That's precisely what I was getting at.  Maybe it makes sense to change, maybe
not, but that's not for this patch to decide as that's a different discussion
from using safe string copying API's.

--
Daniel Gustafsson

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-01T19:47:43Z

    Em seg., 1 de jul. de 2024 às 16:15, Alvaro Herrera <alvherre@alvh.no-ip.org>
escreveu:

> On 2024-Jul-01, Ranier Vilela wrote:
>
> > > -       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.
>
> The bit I don't understand about this discussion is what will happen
> with users that currently have exactly 1024 chars in backup names today.
> With this change, we'll be truncating their names to 1023 chars instead.
> Why would they feel that such change is welcome?
>
Yes of course, I understand.
This can be a problem for users.

best regards,
Ranier Vilela

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-01T19:58:06Z

    Em seg., 1 de jul. de 2024 às 16:20, Daniel Gustafsson <daniel@yesql.se>
escreveu:

> > On 1 Jul 2024, at 21:15, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2024-Jul-01, Ranier Vilela wrote:
>
> >>> -       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.
> >
> > The bit I don't understand about this discussion is what will happen
> > with users that currently have exactly 1024 chars in backup names today.
> > With this change, we'll be truncating their names to 1023 chars instead.
> > Why would they feel that such change is welcome?
>
> That's precisely what I was getting at.  Maybe it makes sense to change,
> maybe
> not, but that's not for this patch to decide as that's a different
> discussion
> from using safe string copying API's.
>
Ok, this is not material for the proposal and can be considered unrelated.

We only have to replace it with strlcpy.

v7 attached.

best regards,
Ranier Vilela

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-01T20:08:33Z

    > On 1 Jul 2024, at 21:58, Ranier Vilela <ranier.vf@gmail.com> wrote:

> We only have to replace it with strlcpy.

Thanks, I'll have a look at applying this in the tomorrow morning.

--
Daniel Gustafsson

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

    Michael Paquier <michael@paquier.xyz> — 2024-07-02T00:33:03Z

    On Mon, Jul 01, 2024 at 09:19:59PM +0200, Daniel Gustafsson wrote:
>> The bit I don't understand about this discussion is what will happen
>> with users that currently have exactly 1024 chars in backup names today.
>> With this change, we'll be truncating their names to 1023 chars instead.
>> Why would they feel that such change is welcome?
> 
> That's precisely what I was getting at.  Maybe it makes sense to change, maybe
> not, but that's not for this patch to decide as that's a different discussion
> from using safe string copying API's.

Yep.  Agreed to keep backward-compatibility here, even if I suspect
there is close to nobody relying on backup label names of this size.
--
Michael

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

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-02T09:44:07Z

    > On 2 Jul 2024, at 02:33, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Mon, Jul 01, 2024 at 09:19:59PM +0200, Daniel Gustafsson wrote:
>>> The bit I don't understand about this discussion is what will happen
>>> with users that currently have exactly 1024 chars in backup names today.
>>> With this change, we'll be truncating their names to 1023 chars instead.
>>> Why would they feel that such change is welcome?
>> 
>> That's precisely what I was getting at.  Maybe it makes sense to change, maybe
>> not, but that's not for this patch to decide as that's a different discussion
>> from using safe string copying API's.
> 
> Yep.  Agreed to keep backward-compatibility here, even if I suspect
> there is close to nobody relying on backup label names of this size.

I suspect so too, and it might be a good project for someone to go over such
buffers to see if there is reason grow or contract.  Either way, pushed the
strlcpy part.

--
Daniel Gustafsson

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-02T11:07:42Z

    Em ter., 2 de jul. de 2024 às 06:44, Daniel Gustafsson <daniel@yesql.se>
escreveu:

> > On 2 Jul 2024, at 02:33, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Mon, Jul 01, 2024 at 09:19:59PM +0200, Daniel Gustafsson wrote:
> >>> The bit I don't understand about this discussion is what will happen
> >>> with users that currently have exactly 1024 chars in backup names
> today.
> >>> With this change, we'll be truncating their names to 1023 chars
> instead.
> >>> Why would they feel that such change is welcome?
> >>
> >> That's precisely what I was getting at.  Maybe it makes sense to
> change, maybe
> >> not, but that's not for this patch to decide as that's a different
> discussion
> >> from using safe string copying API's.
> >
> > Yep.  Agreed to keep backward-compatibility here, even if I suspect
> > there is close to nobody relying on backup label names of this size.
>
> I suspect so too, and it might be a good project for someone to go over
> such
> buffers to see if there is reason grow or contract.  Either way, pushed the
> strlcpy part.
>
Thanks Daniel.

best regards,
Ranier Vilela