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.
Trivial patch attached.
best regards,
Ranier Vilela
Attachments:
avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 330e058c5f..e213329075 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8733,7 +8733,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
+ memcpy(state->name, backupidstr, strlen(backupidstr) + 1);
/*
* Mark backup active in shared memory. We must do full-page WAL writes
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
Attachments:
v1-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=v1-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 330e058c5f..2158256137 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8733,7 +8733,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
+ (void) strlcpy(state->name, backupidstr, sizeof(state->name));
/*
* Mark backup active in shared memory. We must do full-page WAL writes
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
Show quoted text
Attachments:
v2-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=v2-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 330e058c5f..b1c4896c90 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8727,14 +8727,12 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("WAL level not sufficient for making an online backup"),
errhint("\"wal_level\" must be set to \"replica\" or \"logical\" at server start.")));
- if (strlen(backupidstr) > MAXPGPATH)
+ if (strlcpy(state->name, backupidstr, sizeof(state->name)) >= sizeof(state->name))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
-
/*
* Mark backup active in shared memory. We must do full-page WAL writes
* during an on-line backup even if not doing so at other times, because
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
Show quoted text
best regards,
Ranier Vilela
Attachments:
v3-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=v3-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 330e058c5f..b1c4896c90 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8727,14 +8727,12 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("WAL level not sufficient for making an online backup"),
errhint("\"wal_level\" must be set to \"replica\" or \"logical\" at server start.")));
- if (strlen(backupidstr) > MAXPGPATH)
+ if (strlcpy(state->name, backupidstr, sizeof(state->name)) >= sizeof(state->name))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
-
/*
* Mark backup active in shared memory. We must do full-page WAL writes
* during an on-line backup even if not doing so at other times, because
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index c30d4a5991..ec4d7614c2 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -21,8 +21,8 @@
typedef struct BackupState
{
/* Fields saved at backup start */
- /* Backup label name one extra byte for null-termination */
- char name[MAXPGPATH + 1];
+ /* Backup label name */
+ char name[MAXPGPATH];
XLogRecPtr startpoint; /* backup start WAL location */
TimeLineID starttli; /* backup start TLI */
XLogRecPtr checkpointloc; /* last checkpoint location */
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 Vilelabest regards,
Ranier Vilela
--
Yugo NAGATA <nagata@sraoss.co.jp>
Import Notes
Reply to msg id not found: CAEudQAp2L2GBMwiH6+OO5ZAnZmq2zdUEAu1VMpYhdRUWvakEQ@mail.gmail.com.sranhm
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
Show quoted text
So, I think that v3 is ok to fix.
best regards,
Ranier Vilelabest 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 VilelaSo, I think that v3 is ok to fix.
best regards,
Ranier Vilelabest regards,
Ranier Vilela--
Yugo NAGATA <nagata@sraoss.co.jp>
--
Yugo NAGATA <nagata@sraoss.co.jp>
Import Notes
Reply to msg id not found: CAEudQAr2gu477cO-6zY549bxhA9BFH00X32Fex7V6dci+vPDHA@mail.gmail.com.sranhm
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);
longlongpathexample1Next 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>
Import Notes
Reply to msg id not found: CAEudQArgZA3pnrAtG15wDUcW_hLLPv69otLBaiTak9nLZr7MOg@mail.gmail.com.sranhm
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.
--
MichaelEm 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);
longlongpathexample1Next random call:
memcpy(state->name, "longpathexample2", strlen("longpathexample2"));
printf("%s\n", state->name);
longpathexample2ple1In 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);
longlongpathexample1Next random call:
memcpy(state->name, "longpathexample2", strlen("longpathexample2"));
printf("%s\n", state->name);
longpathexample2ple1In 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
Attachments:
v4-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=v4-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8dcdf5a764..01ed7d1ea0 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8736,13 +8736,11 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("WAL level not sufficient for making an online backup"),
errhint("\"wal_level\" must be set to \"replica\" or \"logical\" at server start.")));
- if (strlen(backupidstr) > MAXPGPATH)
+ if (strlcpy(state->name, backupidstr, sizeof(state->name)) >= sizeof(state->name))
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d bytes)",
- MAXPGPATH)));
-
- memcpy(state->name, backupidstr, strlen(backupidstr));
+ MAXPGPATH - 1)));
/*
* Mark backup active in shared memory. We must do full-page WAL writes
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index c30d4a5991..a52345850e 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -21,8 +21,7 @@
typedef struct BackupState
{
/* Fields saved at backup start */
- /* Backup label name one extra byte for null-termination */
- char name[MAXPGPATH + 1];
+ char name[MAXPGPATH];/* backup label name */
XLogRecPtr startpoint; /* backup start WAL location */
TimeLineID starttli; /* backup start TLI */
XLogRecPtr checkpointloc; /* last checkpoint location */
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
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
Attachments:
v5-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/x-patch; name=v5-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d36272ab4f..bfb500aa54 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8742,9 +8742,9 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d bytes)",
- MAXPGPATH)));
+ MAXPGPATH - 1)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
+ (void) strlcpy(state->name, backupidstr, MAXPGPATH))
/*
* Mark backup active in shared memory. We must do full-page WAL writes
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index c30d4a5991..a52345850e 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -21,8 +21,7 @@
typedef struct BackupState
{
/* Fields saved at backup start */
- /* Backup label name one extra byte for null-termination */
- char name[MAXPGPATH + 1];
+ char name[MAXPGPATH];/* backup label name */
XLogRecPtr startpoint; /* backup start WAL location */
TimeLineID starttli; /* backup start TLI */
XLogRecPtr checkpointloc; /* last checkpoint location */
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
Attachments:
v6-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=v6-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d36272ab4f..ecfac05944 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8742,9 +8742,9 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("backup label too long (max %d bytes)",
- MAXPGPATH)));
+ MAXPGPATH - 1)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
+ (void) strlcpy(state->name, backupidstr, MAXPGPATH);
/*
* Mark backup active in shared memory. We must do full-page WAL writes
diff --git a/src/include/access/xlogbackup.h b/src/include/access/xlogbackup.h
index c30d4a5991..a52345850e 100644
--- a/src/include/access/xlogbackup.h
+++ b/src/include/access/xlogbackup.h
@@ -21,8 +21,7 @@
typedef struct BackupState
{
/* Fields saved at backup start */
- /* Backup label name one extra byte for null-termination */
- char name[MAXPGPATH + 1];
+ char name[MAXPGPATH];/* backup label name */
XLogRecPtr startpoint; /* backup start WAL location */
TimeLineID starttli; /* backup start TLI */
XLogRecPtr checkpointloc; /* last checkpoint location */
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
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
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
Attachments:
v7-avoid-incomplete-copy-string-do_pg_backup_start.patchapplication/octet-stream; name=v7-avoid-incomplete-copy-string-do_pg_backup_start.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d36272ab4f..d422b2752f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8744,7 +8744,7 @@ do_pg_backup_start(const char *backupidstr, bool fast, List **tablespaces,
errmsg("backup label too long (max %d bytes)",
MAXPGPATH)));
- memcpy(state->name, backupidstr, strlen(backupidstr));
+ (void) strlcpy(state->name, backupidstr, sizeof(state->name));
/*
* Mark backup active in shared memory. We must do full-page WAL writes
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
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 namestoday.
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