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

On Thu, Mar 6, 2025 at 12:11 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> 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?

Sorry, that's a leftover that I attempted. We should use 'const'.

>
> === 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.
> "

Yes, but we include the suffix when doing sscanf() in many other
places. I don't see the specific benefit of doing it in another way
only in this case.

>
> 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:

Right. How about the following comment?

Bring back the extracted LSN to the snapshot file format and compare
it to the given filename. This check strictly checks if the given filename
follows the format of the snapshot filename.

FYI I've analyzed the idea of extracting LSN from the filename and
bringing it back to the format to validate the given filename. IIUC we
strictly validate the given filename. For example, we accept
'0-ABC.snap' but not '0-0ABC.snap, which is probably fine. Also, I was
concerned that sscanf() could be affected by locale, but it's fine
particularly in this case as we scan only hex numbers.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com