pg_verifybackup: Improve some error handling around strtoul() calls

Three code paths checking the size, timeline ID and system identifier
stored in a manifest now check for an empty value.  Values are always
expected in these parts of a backup banifest.  A couple of tests are
added to validate this behavior

Additionally, precheck_tar_backup_file() checked that "endptr" is NULL.
Based on the C standard, strtoul() never sets an "endptr" to NULL when
given a value (that is the case here), returning a pointer to the
original value if there is nothing to convert.  The pre-tar validation
code is adjusted to do so.

Author: Tristan Partin <tristan@partin.io>
Discussion: https://postgr.es/m/DKBS2Z9CGARC.2T07O6TJYSE8B@partin.io

diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 05385a91e886b3ea1e1ff0ab1c86e48eca612b9c..796eeb6ec0413bec9213bcf15bc799a00bad5e32 100644 (file)
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -961,7 +961,7 @@ precheck_tar_backup_file(verifier_context *context, char *relpath,
                 * Report an error if we didn't consume at least one character, if the
                 * result is 0, or if the value is too large to be a valid OID.
                 */
-               if (suffix == NULL || num <= 0 || num > OID_MAX)
+               if (suffix == relpath || num <= 0 || num > OID_MAX)
                {
                        report_backup_error(context,
                                                                "file \"%s\" is not expected in a tar format backup",

diff --git a/src/bin/pg_verifybackup/t/005_bad_manifest.pl b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
index 0413fea02c35e1270f80ea13038c5cef0e033f0c..bc9fe4a79d710c2aee6227aab882f895a1c54fd5 100644 (file)
--- a/src/bin/pg_verifybackup/t/005_bad_manifest.pl
+++ b/src/bin/pg_verifybackup/t/005_bad_manifest.pl
@@ -39,6 +39,10 @@ test_parse_error('unexpected manifest version', <<EOM);
 {"PostgreSQL-Backup-Manifest-Version": 9876599}
 EOM
 
+test_parse_error('system identifier in manifest not an integer', <<EOM);
+{"PostgreSQL-Backup-Manifest-Version": 1, "System-Identifier": ""}
+EOM
+
 test_parse_error('unexpected scalar', <<EOM);
 {"PostgreSQL-Backup-Manifest-Version": 1, "Files": true}
 EOM
@@ -79,6 +83,12 @@ test_parse_error('file size is not an integer', <<EOM);
 ]}
 EOM
 
+test_parse_error('file size is not an integer', <<EOM);
+{"PostgreSQL-Backup-Manifest-Version": 1, "Files": [
+    {"Path": "x", "Size": ""}
+]}
+EOM
+
 test_parse_error('could not decode file name', <<EOM);
 {"PostgreSQL-Backup-Manifest-Version": 1, "Files": [
     {"Encoded-Path": "123", "Size": 0}
@@ -146,6 +156,12 @@ test_parse_error('timeline is not an integer', <<EOM);
 ]}
 EOM
 
+test_parse_error('timeline is not an integer', <<EOM);
+{"PostgreSQL-Backup-Manifest-Version": 1, "WAL-Ranges": [
+    {"Timeline": "", "Start-LSN": "0/0", "End-LSN": "0/0"}
+]}
+EOM
+
 test_parse_error('could not parse start LSN', <<EOM);
 {"PostgreSQL-Backup-Manifest-Version": 1, "WAL-Ranges": [
     {"Timeline": 1, "Start-LSN": "oops", "End-LSN": "0/0"}

diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 5065c9bf39c03e576b1eb64d46416fe8a9ab4c97..337cae535f4a63032ba13d4c419317e8d0731f77 100644 (file)
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -631,7 +631,7 @@ json_manifest_finalize_system_identifier(JsonManifestParseState *parse)
 
        /* Parse system identifier. */
        system_identifier = strtou64(parse->manifest_system_identifier, &ep, 10);
-       if (*ep)
+       if (ep == parse->manifest_system_identifier || *ep)
                json_manifest_parse_failure(parse->context,
                                                                        "system identifier in manifest not an integer");
 
@@ -688,7 +688,7 @@ json_manifest_finalize_file(JsonManifestParseState *parse)
 
        /* Parse size. */
        size = strtou64(parse->size, &ep, 10);
-       if (*ep)
+       if (ep == parse->size || *ep)
                json_manifest_parse_failure(parse->context,
                                                                        "file size is not an integer");
 
@@ -766,7 +766,7 @@ json_manifest_finalize_wal_range(JsonManifestParseState *parse)
 
        /* Parse timeline. */
        tli = strtoul(parse->timeline, &ep, 10);
-       if (*ep)
+       if (ep == parse->timeline || *ep)
                json_manifest_parse_failure(parse->context,
                                                                        "timeline is not an integer");
        if (!parse_xlogrecptr(&start_lsn, parse->start_lsn))