Re: [musl] RE: [EXTERNAL] Re: [musl] [PATCH] fix avoidable segfault in catclose

[Rich Felker <dalias@libc.org>]

On Sat, Jan 27, 2024 at 03:58:02PM +0300, Alexander Monakov wrote:
> 
> On Sat, 27 Jan 2024, Szabolcs Nagy wrote:
> 
> > > Yes, this - the details aren't particularly interesting but the key is that "invoke UB"
> > > is not the same as "crash/trap".  I'm also contrasting this to the comments in the
> > > glibc wiki and Markus's synopsis (from the earlier email) that "it has been musl policy
> > > to crash on invalid args since the beginning" - in the face of UB, musl (and presumably
> > > also glibc) _doesn't_ crash/trap, nor does it "fail early and catastrophically" it
> > > instead "propagates the UB".  In debug builds these are often equivalent, but the
> > > specific path to UB might not be seen in a debug build, and only be seen in production
> > > where the non-locality of UB effects are at their worst.
> > 
> > i think you are still looking at this the wrong way:
> > 
> > - the original code has ub.
> > - so anything can happen.
> > - whatever libc does, still anything can happen.
> > - adding a check p==-1 in libc does not change anything.
> > (the ub already happens in the caller. a compiler can even remove the
> > call since it can know about catclose semantics.)
> > 
> > given these facts on the theoretical level, we can look pragmatically at
> > the actual transformations a compiler would likely do and we find that
> > an invalid NULL+n dereference in practice is almost surely an immediate
> > crash (on linux with dynamic linking or static linking without lto this
> > is not only likely but actually guaranteed by existing toolchains) which
> > is the best possible outcome for debugging, meanwhile an extra check in
> > libc is worse: the code continues and misbehaves somewhere else.
> 
> I don't think this follows. I believe the suggestion was to have
> 
>     if (catd == (nl_catd)-1) a_crash();
> 
> which is the opposite of "continuing and misbehaving".

Indeed, that was my understanding too. The reason I don't like this is
that it's a lot of spurious code (not in a single place, but if we did
stuff like this consistently everywhere) and on top of that it
actively makes debugging more difficult. You have to trace the flow of
execution from the trapping instruction back to the branch that led to
it and figure out why that was taken rather than seeing the invalid
pointer directly in the instruction operand register (and knowing even
before you see it, e.g. just with strace not even gdb, that the cause
was an invalid pointer).

Rich