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

[Lyndon Brown]

I'm afraid I don't follow the point about "defeating the existing test
case...".
Granted, Qt's string functions are obviously not the only things using
the results of a call to the translation wrappers, but, considering
that passing a NULL to the wrappers is a guaranteed null-deref crash,
and we're not seeing crashes happening all over, clearly users of the
wrappers are typically either cautiously checking for and avoiding
passing NULLs to it, or not having to deal with NULLs.
Again, I'm not sure I can see what you mean in the disabled NLS case
where the wrappers are typically then not called. Currently, if NLS is
enabled and a NULL is passed to the wrapper, we get a null-deref crash,
while if NLS is disabled, then if the wrapper is called anyway (like
with the Qt module's qtr() macro), the NULL is just returned, and if
the wrapper is not called (macro avoids making the call), then the
caller is simply left with the NULL. In these disabled-NLS cases,
either the caller can cope with the NULL (like the Qt function), or
they can't and they null-deref crash.
If we merge this patch to get the wrapper to simply return a given
NULL, then:  1. We get consistent behaviour in the wrappers between
enabled and disabled NLS, (always returning the null).  2. We avoid
unexpected null deref crashes if/when situations like this one occur,
if the caller is able to cope. If the caller can't cope, then we crash
either way. Which I feel is arguably much better behaviour.  3. We
allow callers that can cope fine with a NULL and where NULLs are to be
expected (and don't need to take different action like avoiding
displaying a tooltip), to just let the NULL take a trip through the
wrapper, without the current requirement of having to have a
conditional to catch and avoid passing in NULLs to the wrapper just to
avoid the crash that would otherwise occur.
If we alternatively add an assertion into the wrappers to turn a NULL
into an assert failure rather than a NULL deref, then that would give a
cleaner failure (and greater consistency if done before the NLS define
check). But it leaves inconsistent behaviour between NLS being enabled
and being disabled; Where enabled, we get an assert failure, while when
disabled, if thw wrapper is called anyway (like with Qt code), you we
also thus get an assert failure, but in the typical case where the call
to the wrapper is not made (the macro evaluates to just the NULL), then
either the caller handles the NULL fine, or a null-deref crash occurs.
Thus three possibilities, assert, null-deref, or no-error.
In other words, with the patch applied, it's more likely that in a rare
unexpected situation like what has just occurred that we do not crash,
we just see an empty string where there should be one, and in theory
allow more cases to not bother special-casing NULLS just to avoid
crashing.
If you're suggesting that it's desirable to let the null-deref crash
occur in the wrapper, as a means of discovering problems like what has
occurred here with the missing shorttext, then with respect, I'm really
not sure that I like that idea. I think that had nobody noticed this
problem and it made it into a release, that it would be better form
upon a user happening to click upon one of the few troublesome plugins
in the Qt advanced prefs tree to have encountered an option with no
label, and for that to perhaps go unnoticed/reported for quite some
time, rather than experience a crash.
With respect to the missing option shorttext itself. - I've submitted a
patch separately to add text in the three applicable cases. - If a
modification to vlc_plugin_desc_cb() is also wanted to block plugins
from declaring options without shorttext, then I'm totally on board
with this and can get it done.
On Fri, 2020-09-18 at 10:43 +0300, Rémi Denis-Courmont wrote:
> Hi,
> 
> There are issues in deviating. Practically, it defeats the existing
> test case that looks out for that exact bug class, at least in the
> help texts. Also obviously it won't work if the wrapper is not called
> because NLS is not enabled.
> 
> So what if fromUtf8() can handle NULLs? There are plenty of other
> code paths that get a translated string besides that Qt helper
> function.
> 
> Le 17 septembre 2020 23:46:38 GMT+03:00, Lyndon Brown <
> jnqnfe at gmail.com> a écrit :
> > 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.vlc-devel mailing
> > list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> 
> _______________________________________________vlc-devel mailing
> listTo unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200919/c6783ec5/attachment.html>