From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS22989 209.51.188.0/24 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id ED9F21F47C for ; Sat, 14 Jan 2023 11:01:06 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=clisp.org header.i=@clisp.org header.a=rsa-sha256 header.s=strato-dkim-0002 header.b=e7tbTa/+; dkim-atps=neutral Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pGeHH-0003DT-Qr; Sat, 14 Jan 2023 06:00:47 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pGeHF-0003D7-3l for bug-gnulib@gnu.org; Sat, 14 Jan 2023 06:00:45 -0500 Received: from mo4-p00-ob.smtp.rzone.de ([81.169.146.162]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pGeHB-0008Oo-LI for bug-gnulib@gnu.org; Sat, 14 Jan 2023 06:00:44 -0500 ARC-Seal: i=1; a=rsa-sha256; t=1673694030; cv=none; d=strato.com; s=strato-dkim-0002; b=XFwxDOdUefi/bIF0cZHtouNl9V4RNgETomEgSjEZGhbvJ19hrQUH7Ezxqv/EzbCVpi A/ZutcvzCzQOrNHdWoY5HBGDs0TAD9q8Um/ljnXiWv7JJaPuy+9Ug14/i/bb6jMsxgSk 1usDVWCvc18j17mZeIoskmAzYimZhST02J50bQ8qyEGW+ois73AbBJ0cXl1dHcslGy/4 XUeX+KgGYQ6nflLLkvQXZVtV4yxy539GVfH8W3x8FOnSP+E8nC0g9PrmZwP5sgCazGpj Ve3g9KhmaBjJdDAfl9MJrR3wHLpoUhXyZJZpOlb+NQpQZJyZGVGA/MYVG96cyNHtGzOZ 57qw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; t=1673694030; s=strato-dkim-0002; d=strato.com; h=References:In-Reply-To:Message-ID:Date:Subject:To:From:Cc:Date:From: Subject:Sender; bh=W44llHtvXxzHtRJwSWGsjqww8U2ajphfcEe/XAoQj84=; b=eXIkEJC5jOwRw5paJkfz6YiWD5UDHWhvVJASn8Ye6swmyViWo8Mmb46VNFBZ9ju7Lz 5ASFTBQ+bIDqDovTordw+wKwbt2/zCMbnpUqafftYK3GFi8sb9YuDm3w5+y+WhgZBX9V w/wJuzbiq3bxxbv0gOtHSZkyxXQ6Z8gS+7RhD53L4vbBo0Z//X5xDH58Da145J02auvI lOeCPlBOfevounRvCsq9xWwXeBuXH/DsyG9K1q8MsMCQEUnsAYtzOnkY5Ng8TxBxcS5E Sync/m7zRw4ncjzxEpYsUgy+7iqj6BjHfpENOdd7M38xjY0QKmtHbgM8OYOtr1nFCOEX BwQA== ARC-Authentication-Results: i=1; strato.com; arc=none; dkim=none X-RZG-CLASS-ID: mo00 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1673694030; s=strato-dkim-0002; d=clisp.org; h=References:In-Reply-To:Message-ID:Date:Subject:To:From:Cc:Date:From: Subject:Sender; bh=W44llHtvXxzHtRJwSWGsjqww8U2ajphfcEe/XAoQj84=; b=e7tbTa/+f96X6Wd0tmLk3uHJ834/dxy21Yrow5FF2g36IfSwl+Gv+5BLTO9g4dDchQ FPGDnFVjMgyHX94+XcKEP6AU4o3wqykYoE14sJ3I8AX0qG8J8B/yRxL6gE4sc3jy57wA 2sstlZG9cotMmXSsTv4qvbvji4CQoLBeQe1shbljzrS6GmCZ4wKxnpW7FpbSfPVrrfna ARKlmh6joZITIgGzxeGQuxT+HJCq7g8UllhAeJ2o8fcRCZoMOkLitvUN2CN/NjP2HnQj 1qvLPYctL4yZXYurFKpJtemWY25gJBqHUTwrlSD2JOPMAOhyd66FigTQQkVOh2XUzBT9 YYVQ== X-RZG-AUTH: ":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH0WWb0LN8XZoH94zq68+3cfpPE3fb7Bhi/OceMDKqguFVqjZRzEA==" Received: from nimes.localnet by smtp.strato.de (RZmta 48.6.2 AUTH) with ESMTPSA id I8f358z0EB0UF1T (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Sat, 14 Jan 2023 12:00:30 +0100 (CET) From: Bruno Haible To: bug-gnulib@gnu.org, Paul Eggert Subject: Re: [PATCH 1/4] localename: -Wtautological-pointer-compare Date: Sat, 14 Jan 2023 12:00:29 +0100 Message-ID: <8041574.c9vzh5UkMf@nimes> In-Reply-To: References: <20230113201704.325290-1-eggert@cs.ucla.edu> <17910367.MNNF8PUAaN@nimes> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Received-SPF: none client-ip=81.169.146.162; envelope-from=bruno@clisp.org; helo=mo4-p00-ob.smtp.rzone.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_PASS=-0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Paul Eggert wrote: > > ... starts with an entry check =E2=80=94 which is a good practice [1] >=20 > 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 =3D=3D 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 ... dynamical= ly by the MMU", what the user would see is a SIGSEGV, and would start wond= ering 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= =20 > checked anyway, both statically by the compiler at the call point and=20 > dynamically by the MMU. >=20 > There is a downside of the extra runtime check, in that if static=20 > checking is disabled (a mistake if you ask me, but many people do it),=20 > then the calling code won't immediately crash like it should. Instead,=20 > the function simply sets errno and returns, and the calling code might=20 > go on to do the wrong thing because there's a lot of sloppy calling code= =20 > that either doesn't check errno or that has never been tested to do the=20 > right thing when the function fails. >=20 > So my guess is that for this particular case on practical hosts, the=20 > additional runtime check is more likely to introduce a security bug,=20 > 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 th= an 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