Re: refactoring basebackup.c (zstd workers)

On Mon, Mar 28, 2022 at 03:50:50PM -0400, Robert Haas wrote:
> On Sun, Mar 27, 2022 at 1:47 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Coverity has a nitpick about this:
> >
> > /srv/coverity/git/pgsql-git/postgresql/src/common/backup_compression.c: 194 in parse_bc_specification()
> > 193 /* Advance to next entry and loop around. */
> > >>> CID 1503251: Null pointer dereferences (REVERSE_INULL)
> > >>> Null-checking "vend" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
> > 194 specification = vend == NULL ? kwend + 1 : vend + 1;
> > 195 }
> > 196 }
> >
> > Not sure if you should remove this null-check or add some other ones,
> > but I think you ought to do one or the other.
>
> Yes, I think this is buggy. I think there's only a theoretical bug
> right now, because the only keyword we have is "level" and that
> requires a value. But if I add an example keyword that does not
> require an associated value (as demonstrated in the attached patch)
> and do something like pg_basebackup -cfast -D whatever --compress
> lz4:example, then the present code will dereference "vend" even though
> it's NULL, which is not good. The attached patch also shows how I
> think that should be fixed.
>
> As I hope is apparent, the first hunk of this patch is not for commit,
> and the second hunk is for commit.

Confirmed that it's a real issue with my patch for zstd long match mode. But
you need to specify another option after the value-less flag option for it to
crash.

I suggest to write it differently, as in 0002.

This also fixes some rebase-induced errors with my previous patches, and adds
expect_boolean().