Re: pg_upgrade should truncate/remove its logs before running

On Wed, Jan 19, 2022 at 06:05:40PM -0600, Justin Pryzby wrote:
> I still don't know if it even needs to be configurable.

I want this to be configurable to ease the switch of the pg_upgrade to
TAP, moving the logs into a deterministic temporary location proper to
each run. This makes reporting much easier on failure, with
repeatable tests, and that's why I began poking at this patch first.

> I'm not sure these restrictions are needed ?

This could lead to issues with rmtree() if we are not careful enough,
no? We'd had our deal of argument injections with pg_upgrade commands
in the past (fcd15f1).

> + outputpath = make_absolute_path(log_opts.basedir);
> + if (path_contains_parent_reference(outputpath))
> + pg_fatal("reference to parent directory not allowed\n");
>
> Besides, you're passing the wrong path here.

What would you suggest? I was just looking at that again this
morning, and splitted the logic into two parts for the absolute and
relative path cases, preventing all cases like that, which would be
weird, anyway:
../
../popo
.././
././
/direct/path/to/cwd/
/direct/path/../path/to/cwd/

>> + <command>pg_upgrade</command>, and is be removed after a successful
>
> remove "be"

Fixed.

>> + if (mkdir(log_opts.basedir, S_IRWXU | S_IRWXG | S_IRWXO))
>
> S_IRWXG | S_IRWXO are useless due to the umask, right ?
> Maybe use PG_DIR_MODE_OWNER ?

Hmm. We could just use pg_dir_create_mode, then. See pg_rewind, as
one example. This opens the door for something pluggable to
SetDataDirectoryCreatePerm(), though the original use is kind of
different with data folders.

> You're printing the wrong var. filename_path is not initialized.

Ugh.
--
Michael