bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
From: Bruno Haible <bruno@clisp.org>
To: bug-gnulib@gnu.org, Paul Eggert <eggert@cs.ucla.edu>
Subject: Re: [PATCH 1/4] localename: -Wtautological-pointer-compare
Date: Sat, 14 Jan 2023 12:00:29 +0100	[thread overview]
Message-ID: <8041574.c9vzh5UkMf@nimes> (raw)
In-Reply-To: <b2acc230-4e88-cc46-d08f-366a7affc211@cs.ucla.edu>

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

> 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?


  reply	other threads:[~2023-01-14 11:01 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-13 20:17 [PATCH 1/4] localename: -Wtautological-pointer-compare Paul Eggert
2023-01-13 20:17 ` [PATCH 2/4] Don’t use alloc_size with xlclang 16.1 Paul Eggert
2023-01-13 20:17 ` [PATCH 3/4] assert-h: fix configure comment-out Paul Eggert
2023-01-13 20:17 ` [PATCH 4/4] assert-h: suppress xlclang 16.1 false alarms Paul Eggert
2023-01-13 22:59 ` [PATCH 1/4] localename: -Wtautological-pointer-compare Bruno Haible
2023-01-13 23:36   ` Paul Eggert
2023-01-14 11:00     ` Bruno Haible [this message]
2023-01-15  3:02       ` Paul Eggert
2023-01-15 22:03         ` Bruno Haible
2023-01-16  0:15           ` Paul Eggert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information: https://lists.gnu.org/mailman/listinfo/bug-gnulib

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8041574.c9vzh5UkMf@nimes \
    --to=bruno@clisp.org \
    --cc=bug-gnulib@gnu.org \
    --cc=eggert@cs.ucla.edu \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).