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

[Lyndon Brown]

On Fri, 2020-09-18 at 09:18 +0200, Alexandre Janniaux wrote:
> On Thu, Sep 17, 2020 at 09:46:38PM +0100, Lyndon Brown wrote:
> > On Thu, 2020-09-17 at 22:45 +0300, Rémi Denis-Courmont wrote:
> > > Le torstaina 17. syyskuuta 2020, 22.34.37 EEST Lyndon Brown a
> > > écrit :
> > > > a few plugins have recently introduced options with no
> > > > shorttext,
> > > > resulting in a null dereference crash when their respective
> > > > entries
> > > > are
> > > > selected in the Qt advances prefs tree.
> > > 
> > > Passing NULL for a translated string is not defined. The approach
> > > in
> > > your patch
> > > is not an adequate solution, because it cannot prevent the NULL
> > > dereference
> > > when NLS is disabled.
> > 
> > Not defined? This concerns the VLC wrappers, not the actual gettext
> > functions... Also, if it's UB, then there's no issue deviating, if
> > indeed we are...
> > 
> > With the fix in place, a NULL pointer passed into the wrapper is
> > simply
> > returned, which is the same as when ENABLE_NLS is not defined.
> > 
> > I've tried the fix, and it works, thus the caller (Qt module code
> > in
> > this case) is clearly able to handle the NULL perfectly fine when
> > returned for input to QString::fromUtf8(). I don't see any need to
> > modify the Qt module (or others if they also cope fine) to handle a
> > null in any special way.
> > 
> > I'm afraid  don't see why the fix would be inadequate...
> > 
> > > The NULs need to be replaced with valid strings.
> > 
> > Granted, I'll submit the patches to address that. But things should
> > robustly handle this situation rather than crash.
> 
> Not sure you can robustly manage to have the plugins fixed,
> including future usage, while having a defensive approach.
> 
> Instead, enforcing with an assert might help spotting the
> place where it's not respected.

I'm not totally sure I follow; I wonder whether the discussion is a
little muddled between fixing the issue of options missing shorttext,
and the behaviour of the gettext wrappers in handling a NULL pointer,
not specifically just for the case of the option shorttext, but in the
general use of those wrapper functions.

While the patch happened to fix the crash specifically occurring from
the missing text (with the additional patch adding text also doing so),
and there was some relevance to mentioning that in the commit log and
submission, the focus of this patch is not meant to specifically be a
solution to the missing shorttext crash issue, that's just a side
effect; it's to fix the behaviour of the gettext wrappers, which I feel
are doing the wrong thing in the face of NULLs.

I think I've better explained my feelings on the benefits of returning
NULLs rather than asserting in the other response just sent (with an
assumption that you meant placing an assert in those wrappers).

Essentially: the gettext wrappers are called by many things, and I
think that it's better that should a NULL ever leak through to them
from anywhere they are called in the codebase, that it is better that
they just return that NULL, giving the program a chance to just carry
on - if the caller can cope with a NULL, the typical impact would thus
likely just be an empty label or other output message (not a big deal),
only if it can't cope would it then null-deref crash), vs. the current
situation of a guaranteed crash. An overlooked empty label/message
occurring somewhere is surely better than a program crash trying to
display it.

With respect to the missing shorttext issue specifically and preventing
future mistakes, if we can agree that it is right to enforce that
options *must* have shorttext, that a NULL or empty string is just not
acceptable, then this can be enforced in vlc_plugin_desc_cb().