Re: buildfarm warnings - Mailing list pgsql-hackers

On Thu, Feb 17, 2022 at 12:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > Justin Pryzby <pryzby@telsasoft.com> writes:
> >> pg_basebackup.c:1261:35: warning: storing the address of local variable archive_filename in progress_filename
[-Wdangling-pointer=]
> >> => new in 23a1c6578 - looks like a real error
>
> > I saw that one a few days ago but didn't get around to looking
> > more closely yet.  It does look fishy, but it might be okay
> > depending on when the global variable can be accessed.
>
> I got around to looking at this, and it absolutely is a bug.
> The test scripts don't reach it, because they don't exercise -P
> mode, let alone -P -v which is what you need to get progress_report()
> to try to print the filename.  However, if you modify the test
> to do so, then you find that the output differs depending on
> whether or not you add "progress_filename = NULL;" at the bottom
> of CreateBackupStreamer().  So the variable is continuing to be
> referenced, not only after it goes out of scope within that
> function, but even after the function returns entirely.
> (Interestingly, the output isn't obvious garbage, at least not
> on my machine; so somehow the array's part of the stack is not
> getting overwritten very soon.)
>
> A quick-and-dirty fix is
>
> -               progress_filename = archive_filename;
> +               progress_filename = pg_strdup(archive_filename);
>
> but perhaps this needs more thought.  How long is that filename
> actually reasonable to show in the progress reports?

Man, you couldn't have asked a question that made me any happier.
Preserving that behavior was one of the most annoying parts of this
whole refactoring. The server always sends a series of tar files, but
the pre-existing behavior is that progress reports show the archive
name with -Ft and the name of the archive member with -Fp. I think
that's sort of useful, but it's certainly annoying from an
implementation perspective because we only know the name of the
archive member if we're extracting the tarfile, possibly after
decompressing it, whereas the name of the archive itself is easily
known. This results in annoying gymnastics to try to update the global
variable that we use to store this information from very different
places depending on what the user requested, and evidently I messed
that up, and also, why in the world does this code think that "more
global variables" is the right answer to every problem?

I'm not totally convinced that it's OK to just hit this progress
reporting stuff with a hammer and remove some functionality in the
interest of simplifying the code. But I will shed no tears if that's
the direction other people would like to go.

If not, I think that your quick-and-dirty fix is about right, except
that we probably need to do it every place where we set
progress_filename, and we should arrange to free the memory later.
With -Ft, you won't have enough archives to matter, but with -Fp, you
might have enough archive members to matter.

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