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

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

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

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

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>

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>

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>

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

> 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

> 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

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/

> 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

> 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

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

> 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