Re: rfc/patch: debuginfod client $DEBUGINFOD_PROGRESS env var

[Mark Wielaard <mark@klomp.org>]

Hi Frank,

On Thu, 2019-12-12 at 12:18 -0500, Frank Ch. Eigler wrote:
> > I like this idea but have a bit of a security concern about the ability
> > to set it to any file. If someone manages to set it then they can
> > overwrite anything a process using the library can write to. 
> 
> That's not that serious a category of concern.  Environment variables
> are not under control of untrusted agents.  FWIW, $DEBUGINFOD_CACHE is
> considerably more dangerous in that regard (cache cleaning!).

You have a way to make me even more scared of security issues than less
:)

It would actually be pretty bad if a user made the mistake to set
DEBUGINFOD_CACHE to e.g. their home directory by mistake.

Could we have some extra safeguard there? e.g. If the directory already
exist check whether it is completely empty or if it isn't empty it
contains a cache_clean_interval file? Or at least only delete files
that follow our creation pattern:
 <build-id-hexstring>/[debuginfo|executable|source]?

Doesn't need to be in your patch, but if you think something like that
isn't insane, the paranoid in me would like to add some extra
safeguards to not wipe out someones whole home dir by mistake.

> > I rather it would just use stderr always when set since it is only
> > meant as a human debugging device.
> 
> That default makes sense.  Making it configurable gives a way for a
> larger app to still capture output (by having the client direct it to
> a pipe file descriptor or something), but I'll just hardcode for now
> STDERR_FILENO if the env var is set.

That seems like a good default. Thanks.

> > >     Some larger usage experience (systemtap/kernels) indicates
> > >     the default timeout is too short, so bumped it to 30s.
> > My first thought is that we might need two timeouts. The original 5s
> > timeout is a good inactivity timeout [...] But for larger data we
> > might want a timeout for the total download time, maybe 30 seconds
> > is too low for that. Given how large kernel debuginfo is it might
> > take several minutes.
> 
> OK, redrafting $DEBUGINFOD_TIMEOUT as a two-part variable:
> 
> $DEBUGINFOD_TIMEOUT=x,y
>  x - connection timeout, default 5
>  y - optional, overall timeout, default unlimited

I would call the first progress_timeout since I think it is about
making progress, not just about connecting, but let me comment on the
actual patch. Maybe we actually need 3 values :)

> > [...]
> > This seems to mean that if there is an error then the progress function
> > is never called, is that intended? Also for CURLM_CALL_MULTI_PERFORM we
> > continue/skip the progress function.
> 
> If we have an error outcome, there is no more progress to report, so
> that should be OK.

Yeah, I guess if the first thing is an error, then no progress was ever
really made, so we just never report it. It is just a tiny change in
behavior. Although I doubt anybody would rely on the progress function
being called at least once.

Cheers,

Mark