From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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_MED,SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 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 C822F1F463 for ; Sun, 5 Jan 2020 20:47:08 +0000 (UTC) Received: from localhost ([::1]:45162 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ioCnj-0006x9-4s for normalperson@yhbt.net; Sun, 05 Jan 2020 15:47:07 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:56962) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ioCnd-0006vi-LC for bug-gnulib@gnu.org; Sun, 05 Jan 2020 15:47:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ioCnb-0001wh-Ne for bug-gnulib@gnu.org; Sun, 05 Jan 2020 15:47:01 -0500 Received: from mo6-p00-ob.smtp.rzone.de ([2a01:238:20a:202:5300::12]:27209) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ioCnb-0001r5-0h for bug-gnulib@gnu.org; Sun, 05 Jan 2020 15:46:59 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1578257215; s=strato-dkim-0002; d=clisp.org; h=References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From: X-RZG-CLASS-ID:X-RZG-AUTH:From:Subject:Sender; bh=KH0apfvKQ6GEbhQ+UyKaIofoAOAjxk7j52+GGksYibk=; b=TpbAEd14qc/sgpDJwGvW9v+Us5BZVDpdadRFYDUqxzDUhevgi84PJpl9gN+TJVD0UZ RUugisMT8/KHdP2/viIwvQllRtzcKkCbzfxlK3bEJXyhxWmQCzUGBUWRnRplulbqkVU+ EGe6k4yGHeg6pERCF/Zv3uJz9+OrQiXbiSh5Wxl+wLcPqC4AD18IuzBf1QMpnrMpKnPq 2IhCQDGR1OGgutkCVSTYJSOZEIW57Zq3EvgxF1jCwghWY8hpY9tfi1AkKbmUFSGv1fp9 mhLD+O2DxLHb9kSuFwuk4+p2cedFUzGvKFOpe5ixK34CqF8Ty35dCuTqL519zDY8utZW ukSg== X-RZG-AUTH: ":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH+AHjwLuWOH6fzxfs=" X-RZG-CLASS-ID: mo00 Received: from bruno.haible.de by smtp.strato.de (RZmta 46.1.3 DYNA|AUTH) with ESMTPSA id R06a06w05KksSbh (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (curve X9_62_prime256v1 with 256 ECDH bits, eq. 3072 bits RSA)) (Client did not present a certificate); Sun, 5 Jan 2020 21:46:54 +0100 (CET) From: Bruno Haible To: Jim Meyering Subject: Re: [PATCH] stdlib: avoid canonicalize_file_name contradiction Date: Sun, 05 Jan 2020 21:46:54 +0100 Message-ID: <6027532.2bkh7ZSKZP@omega> User-Agent: KMail/5.1.3 (Linux/4.4.0-170-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: References: <4223468.KeoCiBS5fI@omega> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a01:238:20a:202:5300::12 X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: bug-gnulib@gnu.org Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: "bug-gnulib" Jim Meyering wrote: > > The question is: Is passing NULL to canonicalize_file_name valid? If not, > > then the nonnull attribute should stay. > > > > On one hand, in glibc's stdlib.h we have: > > > > extern char *canonicalize_file_name (const char *__name) > > __THROW __nonnull ((1)) __wur; > > > > On the other hand, in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93156#c3 > > you say "Note that at least some versions of canonicalize_file_name *can* > > take a NULL argument". What are there versions? Even if Cygwin and/or > > Solaris 11 have a function of the same name which allows passing NULL, > > portable code should not pass NULL since that would not work on glibc > > systems. Therefore the diagnostic is useful. > > It is useful indeed. OK, then let's preserve it. > Thanks for working on that. However, it did not help, because at least > on Fedora 30, we're using the system declaration In this case, since the glibc headers (in particular ) do not give us the means to influence what __nonnull expands to, we have no choice than to disable the test when the function comes from the system. We do try to check the system functions just like we check the gnulib functions; this strategy has led us to find numerous libc bugs on various systems. But here, there's no way. Since canonicalize_file_name and ptsname_r are glibc inventions, not POSIX functions, there is no specification that tells what should happen if someone passes a NULL pointer to them; therefore the __nonnull declaration and the effect that it has (from GCC), namely execute arbitrary code, cannot be formally disputed. They can be disputed from the point of view of practical usefulness, though. I've pushed this. I hope it fixes the problem. 2020-01-05 Bruno Haible tests: Avoid GCC over-optimization caused by _GL_ARG_NONNULL attributes. Reported by Jim Meyering in . * lib/stdlib.in.h (GNULIB_defined_canonicalize_file_name): New macro. (GNULIB_defined_ptsname_r): New macro. * tests/test-canonicalize.c (_GL_ARG_NONNULL): Define to empty. (main): Disable the NULL argument test if canonicalize_file_name does not come from gnulib. * tests/test-canonicalize-lgpl.c (_GL_ARG_NONNULL): Define to empty. (main): Disable the NULL argument test if canonicalize_file_name does not come from gnulib. * tests/test-ptsname_r.c (_GL_ARG_NONNULL): Define to empty. (test_errors): Disable the NULL argument test if ptsname_r does not come from gnulib. diff --git a/lib/stdlib.in.h b/lib/stdlib.in.h index e088959..49bbf6f 100644 --- a/lib/stdlib.in.h +++ b/lib/stdlib.in.h @@ -201,6 +201,10 @@ _GL_FUNCDECL_SYS (canonicalize_file_name, char *, (const char *name) # endif _GL_CXXALIAS_SYS (canonicalize_file_name, char *, (const char *name)); # endif +# ifndef GNULIB_defined_canonicalize_file_name +# define GNULIB_defined_canonicalize_file_name \ + (!@HAVE_CANONICALIZE_FILE_NAME@ || @REPLACE_CANONICALIZE_FILE_NAME@) +# endif _GL_CXXALIASWARN (canonicalize_file_name); #elif defined GNULIB_POSIXCHECK # undef canonicalize_file_name @@ -516,6 +520,9 @@ _GL_FUNCDECL_SYS (ptsname_r, int, (int fd, char *buf, size_t len)); # endif _GL_CXXALIAS_SYS (ptsname_r, int, (int fd, char *buf, size_t len)); # endif +# ifndef GNULIB_defined_ptsname_r +# define GNULIB_defined_ptsname_r (!@HAVE_PTSNAME_R@ || @REPLACE_PTSNAME_R@) +# endif _GL_CXXALIASWARN (ptsname_r); #elif defined GNULIB_POSIXCHECK # undef ptsname_r diff --git a/tests/test-canonicalize-lgpl.c b/tests/test-canonicalize-lgpl.c index b46ad87..fb49b20 100644 --- a/tests/test-canonicalize-lgpl.c +++ b/tests/test-canonicalize-lgpl.c @@ -1,4 +1,4 @@ -/* Test of execution of program termination handlers. +/* Test of execution of file name canonicalization. Copyright (C) 2007-2020 Free Software Foundation, Inc. This program is free software: you can redistribute it and/or modify @@ -16,6 +16,11 @@ /* Written by Bruno Haible , 2007. */ +/* Don't use __attribute__ __nonnull__ in this compilation unit. Otherwise gcc + may "optimize" the null_ptr function, when its result gets passed to a + function that has an argument declared as _GL_ARG_NONNULL. */ +#define _GL_ARG_NONNULL(params) + #include #include @@ -66,14 +71,22 @@ main (void) ASSERT (strstr (result, "/" BASE "/tra") == result + strlen (result) - strlen ("/" BASE "/tra")); free (result); + errno = 0; result = canonicalize_file_name (""); ASSERT (result == NULL); ASSERT (errno == ENOENT); + + /* This test works only if the canonicalize_file_name implementation + comes from gnulib. If it comes from libc, we have no way to prevent + gcc from "optimizing" the null_ptr function in invalid ways. See + . */ +#if GNULIB_defined_canonicalize_file_name errno = 0; result = canonicalize_file_name (null_ptr ()); ASSERT (result == NULL); ASSERT (errno == EINVAL); +#endif } /* Check that a non-directory with trailing slash yields NULL. */ diff --git a/tests/test-canonicalize.c b/tests/test-canonicalize.c index e7138a6..81580c5 100644 --- a/tests/test-canonicalize.c +++ b/tests/test-canonicalize.c @@ -16,6 +16,11 @@ /* Written by Bruno Haible , 2007. */ +/* Don't use __attribute__ __nonnull__ in this compilation unit. Otherwise gcc + may "optimize" the null_ptr function, when its result gets passed to a + function that has an argument declared as _GL_ARG_NONNULL. */ +#define _GL_ARG_NONNULL(params) + #include #include "canonicalize.h" @@ -62,22 +67,34 @@ main (void) == result1 + strlen (result1) - strlen ("/" BASE "/tra")); free (result1); free (result2); + errno = 0; result1 = canonicalize_file_name (""); ASSERT (result1 == NULL); ASSERT (errno == ENOENT); + errno = 0; result2 = canonicalize_filename_mode ("", CAN_EXISTING); ASSERT (result2 == NULL); ASSERT (errno == ENOENT); + + /* This test works only if the canonicalize_file_name implementation + comes from gnulib. If it comes from libc, we have no way to prevent + gcc from "optimizing" the null_ptr function in invalid ways. See + . */ +#if GNULIB_defined_canonicalize_file_name errno = 0; result1 = canonicalize_file_name (null_ptr ()); ASSERT (result1 == NULL); ASSERT (errno == EINVAL); +#endif + errno = 0; result2 = canonicalize_filename_mode (NULL, CAN_EXISTING); ASSERT (result2 == NULL); ASSERT (errno == EINVAL); + + errno = 0; result2 = canonicalize_filename_mode (".", CAN_MISSING | CAN_ALL_BUT_LAST); ASSERT (result2 == NULL); ASSERT (errno == EINVAL); diff --git a/tests/test-ptsname_r.c b/tests/test-ptsname_r.c index 19e33ea..24be52f 100644 --- a/tests/test-ptsname_r.c +++ b/tests/test-ptsname_r.c @@ -14,6 +14,11 @@ You should have received a copy of the GNU General Public License along with this program. If not, see . */ +/* Don't use __attribute__ __nonnull__ in this compilation unit. Otherwise gcc + may "optimize" the null_ptr function, when its result gets passed to a + function that has an argument declared as _GL_ARG_NONNULL. */ +#define _GL_ARG_NONNULL(params) + #include #include @@ -84,9 +89,15 @@ test_errors (int fd, const char *slave) } } + /* This test works only if the ptsname_r implementation comes from gnulib. + If it comes from libc, we have no way to prevent gcc from "optimizing" + the null_ptr function in invalid ways. See + . */ +#if GNULIB_defined_ptsname_r result = ptsname_r (fd, null_ptr (), 0); ASSERT (result != 0); ASSERT (result == EINVAL); +#endif } int