Re: BUG #18828: Crash when pg_get_logical_snapshot_meta() passed empty string

Hi,

On Wed, Mar 05, 2025 at 10:42:35PM -0800, Masahiko Sawada wrote:
> On Tue, Mar 4, 2025 at 10:44 PM Bertrand Drouvot
> <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 04, 2025 at 09:45:54PM +0000, Bertrand Drouvot wrote:
> > > Indeed, thanks for looking at it! Fixed in v4 attached. Note that the pfree()
> > > in parse_snapshot_filename() is not needed per say because the function is
> > > currently executed in a short-lived memory context. It's there for safety reason
> > > in case it's called outside those SQL apis in the future.
> >
> > After sleeping on it, PFA a simplified version.
> >
>
> Thank you for updating the patch.
>
> I think we don't need to even do palloc() for the buffer as we can use
> the char[MAXPGPATH] instead.

Sure.

> I've attached the patch to improve the
> parse_snapshot_filename() function and add some regression tests.
> Please review these changes.

Thanks for the patch!

=== 1

-parse_snapshot_filename(const char *filename)
+parse_snapshot_filename(char *filename)

Why?

=== 2

- if (sscanf(filename, "%X-%X", &hi, &lo) != 2)
+ if (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2)

We could replace (sscanf(filename, "%X-%X.snap", &hi, &lo) != 2) with
(sscanf(filename, "%X-%X.foo", &hi, &lo) != 2) and the regression tests would
still pass.

So, I think it's better to remove the .snap here as it could give the "wrong"
impression that it's "useful".

The attached removes the .snap and adds a comment like:

"
* Note: We deliberately don't use "%X-%X.snap" because sscanf only counts
* converted values (%X), not literal text matches.
"

I think it makes sense to document this behavior.

=== 3

+ /*
+ * Bring back the LSN to the snapshot file format and compare
+ * it to the given name to see if the extracted LSN is sane.
+ */
+ sprintf(tmpfname, "%X-%X.snap", hi, lo);
+ if (strcmp(tmpfname, filename) != 0)

The idea was also to ensure that there are no extra characters between the LSN
values and the .snap extension: Adding this as an extra comment in the attached.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com