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.pg@gmail.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