Re: [PATCH 1/4] localename: -Wtautological-pointer-compare

[Bruno Haible <bruno@clisp.org>]

Paul Eggert wrote:
> > ... starts with an entry check — which is a good practice [1]
> 
> It's a good practice in some contexts, bad in others.

It's a good practice at least when
  - the function is non-static (and therefore the callers are not all known),
  - checking the parameters is fast,
  - the code for checking the parameters is small in size, so that it does
    not hamper maintainability.

I wrote

  if (locale == NULL)
    /* Invalid argument.  */
    abort ();

for three reasons:

  * To avoid garbage-in - garbage-out behaviour, which in general encourages
    the presence of bugs.

  * So that when an invalid argument gets passed and the developer happened
    to compile with -g, the debugger will point to the exact line of the
    abort(). When you say "the input should be checked anyway ... dynamically
    by the MMU", what the user would see is a SIGSEGV, and would start wondering
    whether the bug is in his code or in our code.

  * For documentation purposes: So that it's clear that we have considered
    the doc of the function, and NULL at this particular place is invalid.

    (This is not evident: For example, does glibc's error_at_line() support a
    NULL fname argument? Hard to say from
    <https://www.gnu.org/software/libc/manual/html_node/Error-Messages.html>.)

> In this particular case, on practical Gnulib targets the input should be 
> checked anyway, both statically by the compiler at the call point and 
> dynamically by the MMU.
> 
> There is a downside of the extra runtime check, in that if static 
> checking is disabled (a mistake if you ask me, but many people do it), 
> then the calling code won't immediately crash like it should. Instead, 
> the function simply sets errno and returns, and the calling code might 
> go on to do the wrong thing because there's a lot of sloppy calling code 
> that either doesn't check errno or that has never been tested to do the 
> right thing when the function fails.
> 
> So my guess is that for this particular case on practical hosts, the 
> additional runtime check is more likely to introduce a security bug, 
> than to prevent one.

I'm not sure I understand what you write here. Do you mean that adding
  #define _GL_ARG_NONNULL(params)
disables useful static checking on this compilation unit? If so, my
counter-arguments are:

  * A runtime check catches all occasions of passing an invalid argument.
    A static check catches only those where the compiler/tool (gcc,
    clang, -fanalyzer, coverity, ...) has a reason to distinguish NULL and
    non-NULL. For example, when the argument is the value of some variable
    or some function's return, the compiler/tool will not diagnose anything,
    since that would lead to too many diagnostics.

    So, a runtime abort() will catch 100% of the invalid arguments, whereas
    the compiler/tool will catch maybe (wild guess) 25% of them.

    Therefore, eliminating a runtime check to preserve a static check is not
    beneficial. It is more important to preserve the runtime check.

  * We have 15 files in lib/ which disable _GL_ARG_NONNULL. That is less than
    2% of these source files. We don't lose much.

Still, if that is what you are worried about, we can reduce the situations
in which we disable the compiler's NULL argument checking: If I
  - replace all _GL_ARG_NONNULL(params) annotations with
    _GL_FUNC_ARG_NONNULL(func, params)
  - add to localename.c definitions
      #define THIS_COMPILATION_UNIT_DEFINES_duplocale 1
      #define THIS_COMPILATION_UNIT_DEFINES_freelocale 1
  - implement _GL_FUNC_ARG_NONNULL(func, params) so that in this context
    it expands to empty for func being duplocale or freelocale, but to
    _GL_ARG_NONNULL(params) for all other function names,
then most of the __attribute__ __non_null__ annotations are still present,
except for precisely those that we want to eliminate for this precise
compilation unit.

Is that what you are worried about? Should I work on this?

Bruno