Feedback-ID: idd01497b:Fastmail
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Mon, 04 May 2026 15:49:00 +0000
Message-Id: <DIA0EJH2KL60.29M1F14V8I9V7@partin.io>
Cc: "pgsql-hackers" <pgsql-hackers@lists.postgresql.org>
Subject: Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
To: "Amul Sul" <sulamul@gmail.com>
From: "Tristan Partin" <tristan@partin.io>
References: 
 <CAAJ_b97mG=6ybUYE8nQyDfKkhaxTCD8rE8s6L8efNmqHaUYNWQ@mail.gmail.com>
In-Reply-To: 
 <CAAJ_b97mG=6ybUYE8nQyDfKkhaxTCD8rE8s6L8efNmqHaUYNWQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DIA0EJH2KL60.29M1F14V8I9V7%40partin.io>
Precedence: bulk

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, c=
onst char *filename,
>  {
>         char       *ptr =3D *s;
>         int                     prefixlen =3D strlen(prefix);
> +       long            lval;
> +       unsigned long ulval;

Perhaps better variable names would be procNumber and=20
localTransactionId.

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

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

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