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

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

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

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

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

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

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

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

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

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>

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

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

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

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

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

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