Re: refactoring basebackup.c (zstd workers)

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.

--
Robert Haas
EDB: http://www.enterprisedb.com