Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr - Mailing list pgsql-hackers

On Mon Apr 20, 2026 at 12:07 AM CDT, Amul Sul wrote:
> Hi,
>
> The attached patch replaces sscanf with strtol and strtoul in the
> ImportSnapshot helpers (parseIntFromText, parseXidFromText, and
> parseVxidFromText) to improve reliability and efficiency. By utilizing
> the end pointer, we can locate the next line without re-scanning the
> entire string.
>
> Additionally, this change aligns the snapshot code with the rest of
> the Postgres backend, which already favors these functions for safer
> parsing.

Hey Amul,

The patch generally looks good. One comment:

> @@ -1359,17 +1365,36 @@ parseVxidFromText(const char *prefix, char **s, const char *filename,
>  {
>         char       *ptr = *s;
>         int                     prefixlen = strlen(prefix);
> +       long            lval;
> +       unsigned long ulval;

Perhaps better variable names would be procNumber and
localTransactionId.

> +       char       *endptr;
>
>         if (strncmp(ptr, prefix, prefixlen) != 0)
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>                                  errmsg("invalid snapshot data in file \"%s\"", filename)));
>         ptr += prefixlen;
> -       if (sscanf(ptr, "%d/%u", &vxid->procNumber, &vxid->localTransactionId) != 2)
> +
> +       /* Parse procNumber (the signed integer before '/') */
> +       errno = 0;
> +       lval = strtol(ptr, &endptr, 10);
> +       if (endptr == ptr || errno != 0 || lval < INT_MIN || lval > INT_MAX ||
> +               *endptr != '/')
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
>                                  errmsg("invalid snapshot data in file \"%s\"", filename)));
> -       ptr = strchr(ptr, '\n');
> +       vxid->procNumber = (ProcNumber) lval;
> +       ptr = endptr + 1;                       /* skip the '/' separator */
> +
> +       /* Parse localTransactionId (the unsigned integer after '/') */
> +       errno = 0;
> +       ulval = strtoul(ptr, &endptr, 10);
> +       if (endptr == ptr || errno != 0 || ulval > UINT_MAX)
> +               ereport(ERROR,
> +                               (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> +                                errmsg("invalid snapshot data in file \"%s\"", filename)));
> +       vxid->localTransactionId = (LocalTransactionId) ulval;
> +       ptr = strchr(endptr, '\n');
>         if (!ptr)
>                 ereport(ERROR,
>                                 (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),

Otherwise, this looks committable to me. In reviewing, I learned that
sscanf() will parse a string like "   45" as 45, so doesn't seem like we
will have any behavioral differences using strto*().

--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)