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

Hi,

On Tue, Mar 04, 2025 at 10:45:54AM +0000, Bertrand Drouvot wrote:
> Hi,
>
> On Tue, Mar 04, 2025 at 07:38:14AM +0000, Bertrand Drouvot wrote:
> > PFA 0001 a new version to also parse the ".snap".
>
> PFA v3 (a slightly different version) to "correctly" use the new report_error
> introduced in v2.

It looks like that CheckPointSnapBuild() also rely on sscanf() to check
for the snapshot file extension. As seen, up-thread we can't rely on sscanf()
to do so.

Attached a c file to show the "issue":

$ gcc -o sscanf_file_ext sscanf_file_ext.c
$ ./sscanf_file_ext 0-40796E18.snap
File: "0-40796E18.snap"
Parse result: 2 (2 means success)

$ ./sscanf_file_ext 0-40796E18.foo
File: "0-40796E18.foo"
Parse result: 2 (2 means success)

So it looks like that, in reality, in CheckPointSnapBuild() we report a parsing
message and "continue" even if the file name extension is not ".snap".

That's not a big deal because, in pratice, it still removes the .$pid.tmp files
(given they have the "hex" part well formated). But, it does not look like it was
the original intent, so maybe we should also re-think CheckPointSnapBuild() and
open a dedicated thread? (If so, I'm happy to do so).

Note that there might be other places that may need the same kind of attention:
pg_archivecleanup.c?

Regards,

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