Em seg., 1 de jul. de 2024 às 06:20, Daniel Gustafsson <[email protected]>
escreveu:
> > On 27 Jun 2024, at 13:50, Ranier Vilela <[email protected]> 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
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 */
