[vlc-devel] [PATCH] fix null deref crash

[Lyndon Brown]

On Sat, 2020-09-19 at 09:23 +0300, Rémi Denis-Courmont wrote:
> Le lauantaina 19. syyskuuta 2020, 4.24.57 EEST Lyndon Brown a écrit :
> > I'm afraid I don't follow the point about "defeating the existing
> > test
> > case...".
> 
> If the test case no longer detects the problems that it is supposed
> to detect, 
> it's defeated.
> 
> Currently, we get a reliable crash and thus test failure. With an
> assertion, 
> we'd even get a proper abort. With this patch, we get nothing.

But there is no existing test, quite the opposite...

The vlc gettext wrappers dereference the pointer to check if the string
is empty as an optimisation (the only test), but completely ignore the
possibility of the pointer being null in doing that. There is no actual
test there for the situation of the pointer being null and thus no
"test case no longer detecting the problems". The way they are written
looks as though either the author just forgot to check for null, or
never expected a null to ever reach them, which has proven false. If
letting it crash was a deliberate choice, then perhaps a comment should
have been left there to indicate that this was deliberate.

Adding an assert in these functions would add an actual test, and can
improve consistency a bit, but not entirely. Alternatively allowing the
functions to just return a given null to the caller, (1) places the
burden of correctly handling nulls on the caller, where I strongly feel
it should be, and where it clearly is already occurring generally; (2)
makes vlc less likely to crash in unexpected situations (imo it is much
worse to have an obscure unexpected crash than an unnoticed empty
label/message where possible, even if it is helpful to find and fix a
missing string); and (3) hypothetically allows simplifying any callers
which are currently being forced to check for null *only* because the
wrapper can't handle them.

Again in terms of "test case no longer detecting the problems", there
is no existing test that is supposed to catch this at the source, i.e.
vlc_plugin_desc_cb(), where I think that we can agree there should be
one, so again not a problem of an existing test failing, but a simple
lack of one.

Afaik this is the only case of a null unintentionally making it to
these wrapper functions and thus causing a crash, and even in this
case, I'd already both actually noticed the missing strings, and
additionally previously been questioning whether a check should be
added into vlc_plugin_desc_cb(), so it's not like leaving this
unhandled situation in place can even be credited with discovery of
this problem.

Is it really such a good idea to centralise a deliberate crash here to
catch a very rare/unlikely, and not very important mistake, rather than
allowing the wrappers to cleanly handle them, and having a reaosnable
expectation that wherever relevant, the call sites are making suitable
assertions, and/or at the input site. Especially since this is surely
already being done generally, except only in this one case, as far as
we know, and not all that difficult to check that there is not some
other case hidden lying in wait somewhere.

Again, yes we get no helpful crash with this patch to point us to a
problem, but I see that as a good thing. And after all, if the string
in question was really all that important, we're likely to notice in
use and testing should we mistakenly allow a null to slip in somewhere,
and otherwise, like an empty label, we may miss it for some time, but
it's not important, and crashing to highlight it is ugly.

Sorry, I didn't mean to write so much over such a trivial matter.
Although I can appreciate centralised problem catching, I just don't
like it in this case.