Re: logical decoding/replication: new functions pg_ls_logicaldir and pg_ls_replslotdir

On Wed, Nov 17, 2021 at 06:46:47PM +0000, Bossart, Nathan wrote:
> On 10/30/21, 2:36 AM, "Bharath Rupireddy" <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> > I've added 3 functions pg_ls_logicalsnapdir, pg_ls_logicalmapdir,
> > pg_ls_replslotdir, and attached the patch. The sample output looks
> > like [1]. Please review it further.
>
> I took a look at the patch.
>
> + char path[MAXPGPATH + 11];
> + filename = text_to_cstring(filename_t);
> + snprintf(path, sizeof(path), "%s/%s", "pg_replslot", filename);
> + return pg_ls_dir_files(fcinfo, path, false);
>
> Why are you adding 11 to MAXPGPATH here? I would think that MAXPGPATH
> is sufficient.

I suppose it's for "pg_replslot" - but it forgot about the "/".

MAXPGPATH isn't sufficient (even if you add 12), since it's a user-supplied
string. snprintf keeps it from overflowing the buffer, but its return value
isn't checked, so it could (hypothetically) return a result for the wrong slot,
if the slot name were very long, or MAXPGPATH were very short..

+ text *filename_t = PG_GETARG_TEXT_PP(0);

> I think we need to do some additional input validation here. It's
> pretty easy to use this to see the contents of other directories.

Actually, limiting the dir seems like a valid reason to add this function,
since it would allow GRANTing privileges for just those directories.

So now I agree that this patch should be included. But I suggest to add it
after my "ls" patches, which change the output fields, and include directories.
Directories might normally not be present, but an extension might put them
there. (And it may be important to show things that aren't supposed to be
there, too).

--
Justin