<html dir="ltr"><head></head><body style="text-align:left; direction:ltr;"><div>I'm afraid I don't follow the point about "defeating the existing test case...".</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>If we merge this patch to get the wrapper to simply return a given NULL, then:</div><div> 1. We get consistent behaviour in the wrappers between enabled and disabled NLS, (always returning the null).</div><div> 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.</div><div> 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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>With respect to the missing option shorttext itself.</div><div> - I've submitted a patch separately to add text in the three applicable cases.</div><div> - 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.</div><div><br></div><div>On Fri, 2020-09-18 at 10:43 +0300, Rémi Denis-Courmont wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">Hi,<br><br>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.<br><br>So what if fromUtf8() can handle NULLs? There are plenty of other code paths that get a translated string besides that Qt helper function.<br><br><div class="gmail_quote">Le 17 septembre 2020 23:46:38 GMT+03:00, Lyndon Brown <jnqnfe@gmail.com> a écrit :<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>On Thu, 2020-09-17 at 22:45 +0300, Rémi Denis-Courmont wrote:</pre><br><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><pre>Le torstaina 17. syyskuuta 2020, 22.34.37 EEST Lyndon Brown a écrit :</pre><br><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><pre>a few plugins have recently introduced options with no shorttext,</pre><br><pre>resulting in a null dereference crash when their respective entries</pre><br><pre>are</pre><br><pre>selected in the Qt advances prefs tree.</pre><br></blockquote><pre>Passing NULL for a translated string is not defined. The approach in</pre><br><pre>your patch </pre><br><pre>is not an adequate solution, because it cannot prevent the NULL</pre><br><pre>dereference </pre><br><pre>when NLS is disabled.</pre><br></blockquote><br><pre>Not defined? This concerns the VLC wrappers, not the actual gettext</pre><br><pre>functions... Also, if it's UB, then there's no issue deviating, if</pre><br><pre>indeed we are...</pre><br><br><pre>With the fix in place, a NULL pointer passed into the wrapper is simply</pre><br><pre>returned, which is the same as when ENABLE_NLS is not defined.</pre><br><br><pre>I've tried the fix, and it works, thus the caller (Qt module code in</pre><br><pre>this case) is clearly able to handle the NULL perfectly fine when</pre><br><pre>returned for input to QString::fromUtf8(). I don't see any need to</pre><br><pre>modify the Qt module (or others if they also cope fine) to handle a</pre><br><pre>null in any special way.</pre><br><br><pre>I'm afraid don't see why the fix would be inadequate...</pre><br><br><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><pre>The NULs need to be replaced with valid strings.</pre><br></blockquote><br><pre>Granted, I'll submit the patches to address that. But things should</pre><br><pre>robustly handle this situation rather than crash.</pre><pre>vlc-devel mailing list</pre><br><pre>To unsubscribe or modify your subscription options:</pre><br><a href="https://mailman.videolan.org/listinfo/vlc-devel"><pre>https://mailman.videolan.org/listinfo/vlc-devel</pre></a></blockquote></div><br><pre>_______________________________________________</pre><pre>vlc-devel mailing list</pre><pre>To unsubscribe or modify your subscription options:</pre><a href="https://mailman.videolan.org/listinfo/vlc-devel"><pre>https://mailman.videolan.org/listinfo/vlc-devel</pre></a></blockquote></body></html>