[PATCH] m4: Pointer types for pthread_spinlock_init, pthread_mutex_timedlock

[Florian Weimer]

Without this change, these probes fail unconditionally with GCC 14
because they do not use the correct argument types.

---
 m4/pthread-spin.m4| 2 +-
 m4/pthread_mutex_timedlock.m4 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/m4/pthread-spin.m4 b/m4/pthread-spin.m4
index daf9462b52..0b668a15fd 100644
--- a/m4/pthread-spin.m4
+++ b/m4/pthread-spin.m4
@@ -42,7 +42,7 @@ AC_DEFUN([gl_PTHREAD_SPIN],
  [AC_LANG_PROGRAM(
 [[#include 
 ]],
-[[pthread_spinlock_t *lock;
+[[pthread_spinlock_t lock;
   return pthread_spin_init (&lock, 0);
 ]])
  ],
diff --git a/m4/pthread_mutex_timedlock.m4 b/m4/pthread_mutex_timedlock.m4
index 1dc4c8c682..824dab9e24 100644
--- a/m4/pthread_mutex_timedlock.m4
+++ b/m4/pthread_mutex_timedlock.m4
@@ -26,7 +26,7 @@ AC_DEFUN([gl_FUNC_PTHREAD_MUTEX_TIMEDLOCK],
[[#include 
  #include 
]],
-   [[pthread_mutex_t *lock;
+   [[pthread_mutex_t lock;
  return pthread_mutex_timedlock (&lock, (struct timespec *) 0);
]])
 ],

base-commit: 8c7736e4898fb0c177f97b396bbc782a8daa409a

Re: Updating in glibc and gnulib

[Florian Weimer]

* Bruno Haible:

> Florian Weimer wrote:
>> Does gnulib still override  unconditionally?
>
> Gnulib does not override , and never did.

Thanks for looking into this.  gnulib's libc-config.h does this:

| #ifndef __attribute_nonnull__
| /*  either does not exist, or is too old for Gnulib.
|Prepare to include , which is Gnulib's version of a
|more-recent glibc .  */
| …
| /* Include our copy of glibc .  */
| # include 

And as gnulib's  uses the same _SYS_CDEFS_H header guard as
glibc's, that effectively replaces  with gnulib's
.

> That is, when a package that uses Gnulib does
>   #include 
> it will get the  of the system (from glibc, *BSD, Cygwin,
> etc.).

Apparently not if it includes libc-config.h first.

I think what happened is that the order of backporting

commit 0b5ca7c3e551e5502f3be3b06453324fe8604e82
Author: Paul Eggert 
Date:   Tue Sep 21 07:47:45 2021 -0700

regex: copy back from Gnulib

(which brought in __attribute_nonnull__) and

commit a643f60c53876be0d57b4b7373770e6cb356fd13
Author: Siddhesh Poyarekar 
Date:   Wed Oct 20 18:12:41 2021 +0530

Make sure that the fortified function conditionals are constant

was reversed on the glibc 2.34 branch, so the version check based on
__attribute_nonnull__ would signal that system  is too old.
But with the second commit for fortified functions, glibc 2.34 headers
started requiring other macros not present in gnulib's  copy,
so some projects using copied gnulib sources would start to fail.

I backported the regex sync to glibc 2.34 in November, so this should
now be solved because we now define __attribute_nonnull__ even on the
2.34 branch.  I think only the 2.34 branch had this problem.

I think we should have backported the __attribute_nonnull__-defining
commit to glibc 2.34 earlier, when we noticed problems.  Updating the
gnulib-bundled copy only (which is what happened at first) wasn't the
best resolution.

Thanks,
Florian

Updating in glibc and gnulib

[Florian Weimer]

Why does gnulib bundle ?  We edit this file regularly in
glibc.  In the past, some gnulib-using programs supplied their own copy
of  instead, even when building against glibc.  This caused
build failures in the glibc headers because they (quite reasonably)
assumed that  defines the macros for that glibc version.

Does gnulib still override  unconditionally?

A version check will be difficult because sometimes, we have to backport
header fixes to older versions, and that may require adding additional
macros in .

We could move glibc's internal definitions to a new header, reducing
 in scope, but presumably that means gnulib would just
starting bundling that other header, and we would have the same issue
once more.

Thanks,
Florian

Re: xmalloc calling undeclared xalloc_die function

[Florian Weimer]

* Bruno Haible:

> Florian Weimer wrote:
>> lib/xmalloc.c contains this function definition, unconditionally:
>> 
>>   static void * _GL_ATTRIBUTE_PURE
>>   nonnull (void *p)
>>   {
>> if (!p)
>>   xalloc_die ();
>> return p;
>>   }
>> 
>> But the declaration of xalloc_die in lib/xalloc.h is conditional:
>> 
>>   #if GNULIB_XALLOC_DIE
>>   
>>   /* This function is always triggered when memory is exhausted.
>>  It must be defined by the application, either explicitly
>>  or by using gnulib's xalloc-die module.  This is the
>>  function to call when one wants the program to die because of a
>>  memory allocation failure.  */
>>   /*extern*/ _Noreturn void xalloc_die (void);
>>   
>>   #endif /* GNULIB_XALLOC_DIE */
>
> It's conditional on the C macro GNULIB_XALLOC_DIE, which is defined by
> the module 'xalloc-die' (file modules/xalloc-die, line 15).
>
> This conditional was added through
>   <https://lists.gnu.org/archive/html/bug-gnulib/2020-10/msg00140.html>
> 1) to avoid link errors with a compiler that does not eliminate unused
>inline functions,
> 2) to trigger a compilation error instead of a link error or runtime error
>when a packages requerts 'xalloc-die' without 'xalloc' or vice versa
>but then actually uses both.
>
>> I have a package (lbzip2 <https://github.com/kjn/lbzip2/>) which
>> supplies its own definition of xalloc_die, and fails to build due to an
>> undeclared function.
>
> This package calls gnulib-tool like this:
>
>   gnulib-tool --avoid=xalloc-die --add-import pthread utimens warnings \
>   timespec-add timespec-sub dtotimespec stat-time lstat malloc-gnu \
>   fprintf-posix inttypes xalloc largefile gitlog-to-changelog
>
> This means, the module 'xalloc-die' is not included, thus xalloc.h does
> not provide the declaration of xalloc_die().
>
> There are at least three possible fixes:
>
>   * Rather than '--avoid=xalloc-die', the package could override parts
> of the 'xalloc-die' module, as described in
> 
> <https://www.gnu.org/software/gnulib/manual/html_node/Extending-Gnulib.html>.
>
>   * The package could define the C macro GNULIB_XALLOC_DIE.
>
>   * The package could declare xalloc_die().
>
> I would probably pick the second one.

Thanks, this was helpful.  Submitted a patch:

  Define the GNULIB_XALLOC_DIE macro
  <https://github.com/kjn/lbzip2/pull/33>

Florian

Re: -Wlto-type-mismatch warning in error()

[Florian Weimer]

* Bruno Haible:

> Gavin Smith wrote:
>> I expect it is not a gnulib problem.  install-info/install-info.c declares
>> a function called "error" which appears to clash with a function from glibc.
>> ... The "error" module is brought in by "xalloc" (which we
>> ask for explicitly).
>
> Yes, I think the problems already exists without use of Gnulib, as it's
> a conflict between install-info.c and glibc. But with the Gnulib 'error'
> module, the problem becomes bigger.
>
> Namely, see:
>
> $ nm --dynamic /lib/x86_64-linux-gnu/libc.so.6 | grep ' error'
> 001214e0 W error@@GLIBC_2.2.5
> 00121700 W error_at_line@@GLIBC_2.2.5
> 00221484 B error_message_count@@GLIBC_2.2.5
> 00221480 B error_one_per_line@@GLIBC_2.2.5
> 00221488 B error_print_progname@@GLIBC_2.2.5
>
> glibc exports 'error' as a weak symbol. This means, without use of Gnulib,
> the symbol from install-info.c overrides the one from glibc, and there is
> a problem if and only if the 'install-info' program links dynamically
> (or loads via 'dlopen') a shared library which happens to use error()
> and expects the semantics from glibc.

Note that weak vs non-weak does not matter for ELF dynamic linking.  The
definition in the main program always interposes those present in
depended-upon libraries.  This is necessary so that we can add functions
to glibc without an implementation-defined namespace prefix and support
multiple, conflicting standards in parallel (e..g, pthread_create is
reserved in POSIX, but not in C).

The weak flag for symbols is merely and artifact of some internal glibc
symbol management macros.

glibc does not participate in LTO, either, so it shouldn't matter here
at all.

> Whereas with the Gnulib 'error' module, there is a conflict between the
> two global function definitions (with 'T' linkage) in install-info.c and
> in error.c *always*.
>
>> The simplest way to fix this problem would probably be to rename the "error"
>> function in install-info.c.
>
> Yes, or make it 'static' (like Arsen suggested).

Right.

Thanks,
Florian

xmalloc calling undeclared xalloc_die function

[Florian Weimer]

lib/xmalloc.c contains this function definition, unconditionally:

  static void * _GL_ATTRIBUTE_PURE
  nonnull (void *p)
  {
if (!p)
  xalloc_die ();
return p;
  }

But the declaration of xalloc_die in lib/xalloc.h is conditional:

  #if GNULIB_XALLOC_DIE
  
  /* This function is always triggered when memory is exhausted.
 It must be defined by the application, either explicitly
 or by using gnulib's xalloc-die module.  This is the
 function to call when one wants the program to die because of a
 memory allocation failure.  */
  /*extern*/ _Noreturn void xalloc_die (void);
  
  #endif /* GNULIB_XALLOC_DIE */

I have a package (lbzip2 ) which
supplies its own definition of xalloc_die, and fails to build due to an
undeclared function.  So I'm wondering how this is supposed to work.

Thanks,
Florian

Hidden visibility for obstack symbols

[Florian Weimer]

Someone reported that ls in coreutils exported _obstack* symbols.  This
is because ls uses the obstack module from gnulib (I assume).  Due to
the way ELF linking works by default, this promotes the symbols to
global visibility, so that there is a single definition in the entire
process.  This is always a bit iffy (glibc supports it explicitly for
malloc-related symbols, though), but in case of obstack it's really not
great because the gnulib ABI is different from the glibc ABI:

$ gdb /usr/bin/ls
[…]
(gdb) ptype _obstack_begin
type = int (struct obstack *, size_t, size_t, void *(*)(size_t), 
void (*)(void *))
$ gdb /lib64/libc.so.6
[…]
(gdb) ptype _obstack_begin
type = int (struct obstack *, int, int, void *(*)(long), 
void (*)(void *))

This is on x86-64, so the types aren't equivalent.

Would it be possible to give the obstack symbols hidden visibility?

Eventually, we need to fix this on the glibc side, either by deprecating
the interfaces and turning them into a compat interface eventually, or
by switching to size_t in these interfaces.  But in the interim, I think
we should use hidden visibility for the gnulib variants.

(ls loads third-party code via NSS modules, and those modules could
reasonably assume that the glibc obstack symbols follow the glibc ABI.)

Thanks,
Florian

Re: How can Autoconf help with the transition to stricter compilation defaults?

[Florian Weimer]

* Paul Eggert:

> On 2022-11-14 04:41, Aaron Ballman wrote:
>> it's generally a problem when autoconf relies on invalid
>> language constructs
>
> Autoconf *must* rely on invalid language constructs, if only to test
> whether the language constructs work. And Clang therefore must be 
> careful about how it diagnoses invalid constructs. Clang shouldn't
> willy-nilly change the way it reports invalid constructs, as that can 
> break Autoconf.

This is only true for the status quo.  We could finally band together
and define an interface between autoconf and the toolchain that avoids
feature probing through source code fragments for common cases.  It
might make configure scripts to run quite a bit faster, too.

That being said, system compilers need to be careful when turning
warnings into errors by default, but that doesn't mean we should never
do such changes, particularly when we know based on interactions with
programmers that the warnings are not sufficient for avoiding wasted
time.

Thanks,
Florian

Re: On time64 and Large File Support

[Florian Weimer]

* Sam James:

>> On 11 Nov 2022, at 09:19, Florian Weimer  wrote:
>> 
>> * Sam James:
>> 
>>> In Gentoo, we've been planning out what we should do for time64 on
>>> glibc [0] and concluded that we need some support in glibc for a newer
>>> option. I'll outline why below.
>>> 
>>> Proposal: glibc gains two new build-time configure options:
>>> * --enable-hard-time64
>>> * --enable-hard-lfs
>> 
>> We should define new target triplets for this if it's really required.
>
> I hadn't considered that option. I'll reflect on it. Please let me know
> if you have further thoughts on this.
>
> But that said, these binaries are broken anyway in 2038?

No, I expect users to run them in time-shifted VMs or containers.
Wrong dates are better than no ability to run anything at all.

And whoever can recompile to switch to time64 can just recompile for a
64-bit target.  There are some embedded users that stick to 32-bit for
cost savings, but I think the cost allocation is quite wrong: They save
a bit on per-unit costs, but they do not really contribute back to GNU
(and most don't even use glibc, although there is some use out there).

>> We need to support legacy binaries on i386.  Few libraries are
>> explicitly dual-ABI.  Whether it's safe to switch libraries above glibc
>> to LFS or time64 needs to be evaluated on a per-library basis.  For most
>> distributions, no one is going to do that work, and we have to stick to
>> whathever we are building today.
>
> While I agree, I don't think it's as well-known that it should be that
> these are ABI breaking and require inspection. It's being done ad-hoc
> or in many cases, not at all.
>
> Part of the use of this thread is, if nothing else, we can show upstreams
> and other distros It if they're confused.
>
> It's very easy to miss that a package has started enabling LFS
> and then your opportunity to catch the ABI breakage is gone.
>
> It doesn't help that I (and I suspect most distribution maintainers)
> do all development on x86_64 and hence even ABI diffing isn't
> going to notice. You have to specifically diff the build system, which I
> do, but it's not easy if it's buried deep within gnulib or something.

I really assumed that setting the default in glibc would solve this for
everyone: binary distributions keep using time32, and source-based
embedded distributions can switch to time64 if they want to. *sigh*

I mean, we have things like more compact stack usage through certain
ABI-breaking GCC options.  The kernel can use those safely, but few
people attempt to apply them to userspace.  There, having the right
default in the toolchain is sufficient.  I didn't expect time64 to be
different.

Thanks,
Florian

Re: scratch_buffer.h, scratch_buffer_dupfree.c sync

[Florian Weimer]

* Paul Eggert:

> What problems do you see with the interfaces, and are there efforts to
> come up with a better API? The need is there in GNU apps, each of
> which tends to roll its own code here.

dynarray has an aliasing violation in its implementation.  The embedded
pointer should be void * (not DYNARRAY_ELEMENT *), with casts added in
the generated code, so that the out-of-line code can use the correct
types.

Defining the element free function has a trap for the unwary programmer:
it's easy to miss the required pointer indirection.  I don't know a good
fix for that.

I view dynarray as a stop-gap until we can use a C++ template inside
glibc.  We won't be able to use std::vector because of gaps in memory
allocation failure handling, but application code interested in basic
type-generic data structures should really use libstdc++ and not
preprocessor hacks like dynarray.  With such wide use of xmalloc &
friends, the memory allocation issue would impact few applications.

The scratch buffer interface is mainly intended as a helper for calling
NSS functions because of their highly problematic buffer interface: it's
not just that the caller allocates, the callee does not provide any size
hint at all if the provided buffer is not large enough.  So the only
thing you can do is keep retrying with larger and larger buffers.  No
one should create interfaces like that.

One could argue that scratch buffers are needed because the NSS
interfaces exist today, but from a glibc perspective, I think we should
provide replacements for the problematic NSS functions that are
significantly easier to use, rather than relying on developers to put
something together using scratch buffers.  Maybe it's sufficient to make
the original functions like getpwuid thread-safe, although such
functions manipulating global state wouldn't be really library-safe.
(You wouldn't want to call getpwuid in a library because it might
clobber another getpwuid result still in use elsewhere, but that's an
issue that is present with or without thread safety.)

I added scratch_buffer_grow_preserve and scratch_buffer_set_array_size
as an afterthought.  Conceptually, they are unrelated to the NSS usage.
They predate dynarrays.  If dynarrays were easier to define, we should
have switched over to them because they have implicit overflow checking
for allocation sizes.

Thanks,
Florian

Re: scratch_buffer.h, scratch_buffer_dupfree.c sync

[Florian Weimer]

* Bruno Haible:

> But when it comes to scratch_buffer or dynarray code, this week's experience
> has shown that we cannot count on as much care for Gnulib users. Rather,
> the mindset on the glibc side seems to be: "This is glibc internal code;
> we can refactor / reshuffle / trim it as we like." [1][2]
>
> So, if we want to offer a Gnulib module from glibc-internal code, it would
> be our job to maintain compatibility across glibc's refactorings. In this
> particular case, it would have meant to add the scratch_buffer_dupfree.c
> as a Gnulib-owned source file. But it the long run, this is going to be
> a growing maintenance effort. (We have a similar situation: gettext's libintl
> is derived from glibc/intl/, and it's a continuing effort to keep the two
> more or less in sync.)
>
> Therefore I agree with what you did yesterday: remove scratch_buffer_dupfree.c
> from Gnulib, since glibc dropped it.
>
> But it means that we cannot promise that these .h files are even remotely
> stable APIs.

I must say I was surprised to see dynarray and scratch_buffer end up in
gnulib.  I never intended them to escape this way from glibc.  The
interfaces and their implementation are problematic in some ways, and I
can't recommend them for general use.

Thanks,
Florian

Re: [patch] glob: resolve DT_UNKNOWN via is_dir

[Florian Weimer]

* DJ Delorie:

> Florian Weimer  writes:
>>> + if (fullpath == NULL)
>>> +   /* This matches old behavior wrt DT_UNKNOWN.  */
>>> +   break;
>>
>> Shouldn't this report memory allocation failure to the caller?
>
> We could easily replace that break with a "goto memory_error" but that
> changes the new logic from "better hint" to "mandatory function".

I thought the bug is that it's not actually a hint?  That DT_UNKNOWN
here leads to incorrect results in glob?

Thanks,
Florian

Re: [patch] glob: resolve DT_UNKNOWN via is_dir

[Florian Weimer]

* DJ Delorie:

> diff --git a/lib/glob.c b/lib/glob.c
> index 22c459574..d0521bb4a 100644
> --- a/lib/glob.c
> +++ b/lib/glob.c
> @@ -1381,7 +1381,26 @@ glob_in_dir (const char *pattern, const char 
> *directory, int flags,
>if (flags & GLOB_ONLYDIR)
>  switch (readdir_result_type (d))
>{
> -  case DT_DIR: case DT_LNK: case DT_UNKNOWN: break;
> +  case DT_DIR: case DT_LNK: break;
> +   case DT_UNKNOWN:
> + {
> +   /* The filesystem was too lazy to give us a hint,
> +  so we have to do it the hard way.  */
> +   char *fullpath, *p;
> +   bool isdir;
> +   fullpath = malloc (strlen (directory) + strlen (d.name) + 
> 2);
> +   if (fullpath == NULL)
> + /* This matches old behavior wrt DT_UNKNOWN.  */
> + break;

Shouldn't this report memory allocation failure to the caller?

Thanks,
Florian

Re: [PATCH 2/2] posix: Remove alloca usage for internal fnmatch implementation

[Florian Weimer]

* Adhemerval Zanella:

> On 08/03/2021 09:59, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> -else if (*p == L_('|'))
>>> +else if (*p == L_(')') || *p == L_('|'))
>>>{
>>>  if (level == 0)
>>>{
>>> -NEW_PATTERN;
>>> -startp = p + 1;
>>> +size_t slen = opt == L_('?') || opt == L_('@')
>>> + ? pattern_len : p - startp + 1;
>>> +CHAR *newp = malloc (slen * sizeof (CHAR));
>>> +if (newp != NULL)
>>> +  {
>>> +*((CHAR *) MEMPCPY (newp, startp, p - startp)) = L_('\0');
>>> +PASTE (PATTERN_PREFIX,_add) (&list, newp);
>>> +  }
>>> +if (newp == NULL || PASTE (PATTERN_PREFIX, _has_failed) 
>>> (&list))
>>> +  {
>>> +retval = -2;
>>> +goto out;
>>> +  }
>>> +
>>> +if (*p == L_('|'))
>>> +  startp = p + 1;
>>>}
>> 
>> slen seems to be the wrong variable name.  But I don't know wh the
>> original code computes plen conditionally and then uses p - startp
>> unconditionally.  That seems wrong.  The discrepancy goes back to
>> 821a6bb4360.  Do you see a case where the difference matters?
>> 
>> The == 0 checks for the recursive FCT calls are wrong because they treat
>> match failure the same as OOM and other errors (the -2 return value),
>> but that also is a pre-existing issue.
>> 
>> The conversation itself appears to be faithful.
>
> Hi Florian,
>
> I noted this patch [1] is marked accepted, was you the one that
> accepted it? In any case, are you still ok with the change?
>
>
> [1] 
> https://patchwork.sourceware.org/project/glibc/patch/20210202130804.1920933-2-adhemerval.zane...@linaro.org/

I think all the issues I identified are pre-existing, and as I said, the
conversion to remove alloca appears to be correct.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Joseph Myers:

> On Sat, 17 Jul 2021, Bruno Haible wrote:
>
>> 2) /usr/include/gnu/lib-names.h still defines LIBPTHREAD_SO.
>>How about not defining LIBPTHREAD_SO, since linking with it is supposed
>>to be a no-op in these newer glibc versions?
>
> I think LIBPTHREAD_SO is really for use with dlopen (followed by e.g. 
> using dlsym to look up a function by name at runtime), not linking against 
> (in general you need to link against the *.so name which might be a linker 
> script, not directly against the shared library's SONAME).
>
> So if there's any change regarding LIBPTHREAD_SO, I think the natural one 
> would be to define it to LIBC_SO (I hope the dlopen/dlsym case works 
> regardless of whether that change is made or not).

That is in an interesting idea.  I like it.

It doesn't help with Bruno's use case, detecting the integrated
libpthread with the preprocessor.

Carlos, do you think we can still slip in a definition of
PTHREAD_IN_LIBC in  (for __USE_GNU)?

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Bruno Haible:

> Florian Weimer wrote on 2021-04-28:
>> However, you should really remove those weak symbol
>> hacks.  They won't have any effect for glibc 2.34
>
> I'm trying to do this now. But what is the right condition?
>
> 1) I understand that it's only for glibc >= 2.34 on Linux (NPTL),
>right? Not on Hurd (HTL)?

Yes, Hurd hasn't been integrated.

> 2) /usr/include/gnu/lib-names.h still defines LIBPTHREAD_SO.
>How about not defining LIBPTHREAD_SO, since linking with it is supposed
>to be a no-op in these newer glibc versions?

I'm not sure if this is the right way, given that the file still exists
for all currently supported targets.

An alternative would be to add a macro to , such as
PTHREAD_IN_LIBC.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Matthias Klose:

> On 7/12/21 5:03 PM, Florian Weimer via Binutils wrote:
>> * Michael Hudson-Doyle:
>> 
>>> Did this thread ever reach a conclusion? I'm testing a snapshot of
>>> glibc 2.34 in ubuntu and running into this issue -- bison segfaults on
>>> startup on ppc64el.
>
>> We rebuilt bison and a couple of other packages
>
> do you have a list of these packages?

I rebuilt everything that had a weak symbol reference to
pthread_mutexattr_gettype or thread_exit because those two symbols are
used for the single-thread optimization in current gnulib.  The presence
of these symbols largely depends on at which point the upstream sources
you use last imported the relevant modules from gnulib, so it's probably
best to check each distribution individually.  According to my notes,
for us, that was bison, findutils, nano, and gnulib itself.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Michael Hudson-Doyle:

> Did this thread ever reach a conclusion? I'm testing a snapshot of
> glibc 2.34 in ubuntu and running into this issue -- bison segfaults on
> startup on ppc64el.

For us it got resolved with a binutils fix:

commit b293661219c36e72acb80502a86b51160bb88cfd
Author: Alan Modra 
Date:   Mon May 3 10:03:06 2021 +0930

PPC: ensure_undef_dynamic on weak undef only in plt

It's slightly weird to have a call to a weak function not protected by
a test of that function being non-NULL, but the non-NULL test might be
covered by a test of another function.  For example:
  if (func1)
{
  func1 ();
  func2 ();
}
where func2 is known to exist if func1 exists.

* elf32-ppc.c (allocate_dynrelocs): Call ensure_undef_dynamic for
weak undefined symols that only appear on PLT relocs.
* elf64-ppc.c (allocate_dynrelocs): Likewise.

We rebuilt bison and a couple of other packages that looked like it
would be affected by this before putting glibc 2.34 snapshots into the
buildroot, and that worked quite well for us.  (Thanks to Andreas Schwab
for identifying the issue so early.)

We don't know yet whether there are user binaries out there which will
be incompatible with glibc 2.34.  I have posted a glibc patch which
alters symbol resolution to increase compatibility with old binaries (so
it's technically feasible to get this working again), but in the review
discussion, I was asked to break *more* older binaries, including every
i386 binary from the late 1990s/early 2000s, so I dropped that patch and
did not pursue this approach further.  But I guess we can get back to it
if feedback from end users indicates that the current approach doesn't
work for them.

Thanks,
Florian

Re: [PATCH] year2038: support glibc 2.34 _TIME_BITS=64

[Florian Weimer]

* Paul Eggert:

> On 7/7/21 1:45 AM, Florian Weimer wrote:
>
>> Y2038 support requires recompilation.  If you are able to do that, why
>> not recompile for a 64-bit architecture?
>
> Doesn't this argue against _TIME_BITS=64 in general? It seems to be
> saying that one should just recompile for 64-bit, and never use 
> _TIME_BITS=64.

I think it does, but apparently 32-bit Arm is an outlier, related to
DRAM sizes.  I'm still not convinced that glibc needs to support that,
but the community wasn't opposed to it.

>> This probably needs per-package/component work to enable dual ABI,
>> similar to what glibc did for its time_t interfaces
>> I don't expect many upstreams to support this effort.
>
> Agreed.
>> Two separate i386 ports seem to require the least human
>> resources to maintain.
>
> That's a reasonable approach and if people want to do that they can,
> even with the latest Gnulib and the next version of Glibc.
>
> However, people who want to run old binaries will surely stick to the
> 32-bit-time_t i386 port, which means they won't use the 64-bit-time_t 
> i386 port. So it's not clear to me that they will cotton to this approach.

Sorry, I don't understand.  Which approach?

I expect the legacy i386 port to be the main one.

Thanks,
Florian

Re: [PATCH] year2038: support glibc 2.34 _TIME_BITS=64

[Florian Weimer]

* Bruno Haible:

> Florian Weimer wrote:
>
>> 64-bit file offsets enabled real use cases.
>
> Year 2038 is also a real use-case. It is not so rare that machines are
> being used for 15 years. (I still occasionally use a 14-years old
> computer, and had a washing machine that lasted 25 years.)
> Year 2038 is less than 17 years away. So, it is time to do something for
> year 2038 now, not in five years.

Y2038 support requires recompilation.  If you are able to do that, why
not recompile for a 64-bit architecture?

>> I assume GNU clisp (at least in a full build) need to link to some
>> system libraries which are not dual ABI (and probably never will be).
>> If gnulib forces the use of time64 mode, then it creates a push towards
>> time64 mode in those libraries, too.  At that point, these libraries
>> will no longer be usable for running older binaries (in at least some
>> cases; in others, the time_t symbols are not actually used).
>> ...
>> gnulib is pushing things in one particular direction, a
>> direction ...
>
> Let me try to summarize your arguments, the way I understand them.
>
>   1) The ability to run older binaries is essential for nearly all
>  distros.
>
>   2) On i386, 32-bit time_t and 64-bit time_t are not binary compatible,
>  when used in the public API of a shared library. Assume an existing
>  old binary relies on /usr/lib/libfoo.so.5 and uses its API with
>  32-bit time_t assumption. Then this library must stay in place with
>  the same API.
>
>   3) The distribution can provide a libfoo.so compiled with 64-bit
>  time_t, but it MUST reside in a different file.

I think there is also the possibility of a dual ABI, see below.

> Pieces that are missing, AFAICS, are:
>
>  A) Possibly some glibc "magic" with shared library versioning would
> make this situation simpler? Or is the combination of ldconfig and
> LD_LIBRARY_PATH etc. sufficient?

This probably needs per-package/component work to enable dual ABI,
similar to what glibc did for its time_t interfaces.  There is no linker
magic involved, it's either symbol redirects (using a GCC extension) or
a preprocessor macro.  A dual ABI avoids the need for new soname and the
introduction of symbol versioning (so that the object can be loaded
twice into the same process with different ABIs).

I don't expect many upstreams to support this effort.

>  B) A writeup for distributors, what is the recommended way to handle
> the situation.
> There are several _possible_ ways to handle it. But Linux distros
> aim at being compatible at the binary level, and that requires
> a _common_ approch among distros. IMO, the Linux Standard Base (LSB)
> is the forum where such things should be standardized.
> Have the LSB people already been involved in the discussion?

LSB is quite dead, and it never covered the interesting packages anyway.

I can see distributions building 32-bit Arm and a *new*, separate
variant of i386 for 64-bit time_t, and the original i386 port remains at
32-bit.  The new i386 port would have a glibc that defaults to 64-bit
time_t.  Two separate i386 ports seem to require the least human
resources to maintain.  If that's the chosen approach, gnulib should
just use whatever the default time_t size is, and not attempt to
override it.

Thanks,
Florian

Re: [PATCH] year2038: support glibc 2.34 _TIME_BITS=64

[Florian Weimer]

* Bruno Haible:

> Hi Florian,
>
>> > In glibc 2.34 on Linux kernels where time_t is traditionally 32-bit,
>> > defining _FILE_OFFSET_BITS=64 and _TIME_BITS=64 makes time_t 64-bit.
>> > Apps must define both macros.  Gnulib applications that use either
>> > the largefile or the year2038 modules will want this behavior;
>> > largefile because it deals with the off_t and ino_t components of
>> > struct stat already, and so should also deal with time_t.
>> 
>> Won't this be a very disruptive change to distributions, whose system
>> libraries have not switched to 64-bit time_t on 32-bit?
>> 
>> gnulib should not try to force a different distribution default.  I'm
>> worried that this will lead to distributions abandoning 32-bit i386
>> support altogether because the support cost is too high—and you can't
>> even run legacy binaries anymore.
>
> I don't understand your points regarding "very disruptive change",
> "distribution default", and "can't even run legacy binaries". Probably
> you have something in mind that differs from my understanding.
>
> In my understanding, a change like this one propagates to the tarballs
> that make use of Gnulib. For example, GNU tar will, starting with the
> next version, contain logic that has the effect of adding
>   #define _TIME_BITS 64
> to the config.h of that package. Thus, GNU tar and GNU mt will, on
> glibc ≥ 2.34 systems, be internally using a different time_t type than
> programs that don't use Gnulib (e.g. util-linux) and programs that use
> older versions of Gnulib (e.g. GNU clisp).

I assume GNU clisp (at least in a full build) need to link to some
system libraries which are not dual ABI (and probably never will be).
If gnulib forces the use of time64 mode, then it creates a push towards
time64 mode in those libraries, too.  At that point, these libraries
will no longer be usable for running older binaries (in at least some
cases; in others, the time_t symbols are not actually used).

> From the perspective of the distributions, this is a no-op, IMO.

It is not, gnulib is pushing things in one particular direction, a
direction that not everyone working on the GNU toolchain agrees with.

> The only problem that I see is with *libraries* that have an API that
> references the time_t type. It is well-known that when a library
>   - references off_t or 'struct stat' in its API, and
>   - was built with AC_SYS_LARGEFILE in effect,
> the packages that use this library also have to be built with
> AC_SYS_LARGEFILE. This has caused problems in the past, when
> _FILE_OFFSET_BITS=64 was introduced (ca. 2000-2005).

There are some major differences to _FILE_OFFSET_BITS=64:

There weren't 20+ years of backwards compatibility in 2005, with a large
set of legacy applications.  Today, i386 without the ability run legacy
programs is fairly useless and perhaps not worth maintaining.

64-bit file offsets enabled real use cases.  People usually couldn't
address those in another way, given that LP64 CPUs and userspace wasn't
yet mainstream.

> I don't see big problems with distribution vendors, since 56%
> of the distributions have already abandoned i386 ports by now [1],
> and more will follow suit. The rest of the distros can easily
> add -D_TIME_BITS=64 to their common compilation flags.

I think those 56% count installation images, not installable i386
library packages on x86-64 systems.

Thanks,
Florian

Re: [PATCH] year2038: support glibc 2.34 _TIME_BITS=64

[Florian Weimer]

* Paul Eggert:

> In glibc 2.34 on Linux kernels where time_t is traditionally 32-bit,
> defining _FILE_OFFSET_BITS=64 and _TIME_BITS=64 makes time_t 64-bit.
> Apps must define both macros.  Gnulib applications that use either
> the largefile or the year2038 modules will want this behavior;
> largefile because it deals with the off_t and ino_t components of
> struct stat already, and so should also deal with time_t.

Won't this be a very disruptive change to distributions, whose system
libraries have not switched to 64-bit time_t on 32-bit?

gnulib should not try to force a different distribution default.  I'm
worried that this will lead to distributions abandoning 32-bit i386
support altogether because the support cost is too high—and you can't
even run legacy binaries anymore.

Thanks,
Florian

Re: an old fstatat bug

[Florian Weimer]

* Paul Eggert:

>> +This function does not fail when the second argument is an empty string
>> +on some platforms:
>> +glibc 2.7.
>
> If AT_EMPTY_PATH is specified, fstatat is documented to behave that
> way with an empty string. So possibly this behavior is also
> kernel-dependent (AT_EMPTY_PATH was introduced in Linux 2.6.39, maybe
> the kernels are buggy before that?). Not sure whether this is all
> worth mentioning.

The behavior probably changed when plain fstat accepted O_PATH
descriptors.  It's not necessary anymore to use fstatat and
AT_EMPTY_PATH with O_PATH descriptors on current Linux.

That would be:

commit 55815f70147dcfa3ead5738fd56d3574e2e3c1c2
Author: Linus Torvalds 
Date:   Fri Sep 14 14:48:21 2012 -0700

vfs: make O_PATH file descriptors usable for 'fstat()'

We already use them for openat() and friends, but fstat() also wants to
be able to use O_PATH file descriptors.  This should make it more
directly comparable to the O_SEARCH of Solaris.

Note that you could already do the same thing with "fstatat()" and an
empty path, but just doing "fstat()" directly is simpler and faster, so
there is no reason not to just allow it directly.

See also commit 332a2e1244bd, which did the same thing for fchdir, for
the same reasons.

Reported-by: ольга крыжановская 
Cc: Al Viro 
Cc: sta...@kernel.org# O_PATH introduced in 3.0+
Signed-off-by: Linus Torvalds 

Thanks,
Florian

glibc support for allocating alternate signal stacks

[Florian Weimer]

I've posted a proposal to provide an interface for allocating stacks
in glibc:

  Misc: Add  and the cstack_* family of functions
  

This is based on our observation that many applications use static
buffers for alternate stacks, which is not thread-safe.

I'd appreciate If you could comment here or on libc-alpha based on your
experience with alternate signal stacks.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Bruno Haible:

>> I spent today on coming up with a workaround in glibc.
>
> These are the workarounds I can see:
>   - Delay the planned changes in glibc by 5 years or so, to minimize
> the number of binaries out there that would crash. (Probably not what
> you want.)

Nah, those binaries won't go away in just five years.

>   - Change glibc's ld.so to deal with the binaries that are out there
> and that have been produced by existing binutils (with or without the
> patches that H.J. Lu listed).

This is what I've tried to implement:

  [RFC] elf: Implement filtering of symbols historically defined in libpthread
  

>   - Play tricks with the preprocessor, such as
> '#define pthread_create pthread_create_in_libc'. (Probably not POSIX
> compliant.)

It doesn't solve the problem, even for new binaries.

>   - Make use of symbol versioning. Symbol versioning was invented to
> allow making big changes to libc without breaking binary backward
> compatibility. (I don't know about the interplay between weak and
> versioned symbols.)

If the symbol is weak and undefined at build time, there is no symbol
version attached to it.  My patch uses that to make sure the filtering
does not happen on the fast path (because symbols bound to libc.so.6
usually have versions), but I don't think symbol versioning is all that
useful in this context.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Bruno Haible:

> So, in the normal cases (link with '-lpthread', link without '-lpthread',
> and even with dlopen()), everything will work fine. The only problematic
> case thus is the the use of LD_PRELOAD. Right?

LD_PRELOAD and glibc 2.34 as originally planned.

> I think few packages in a distro will be affected. And few users are
> using LD_PRELOAD on their own, because since the time when glibc
> started to use 'internal' calls to system calls where possible, there
> are not a lot of uses of LD_PRELOAD that still work.

We get the occasional bug report when these things break.  We have not
seen much of that yet because our gnulib-using programs are still at
older versions for most of our users.

Here's an example of such a bug report, although not for libpthread:

  powerpc: libc segfaults when LD_PRELOADed with libgcc
  

I think in this bug, libc.so.6 was invoked during some build process
(which could easily run bison as well, so it has to work with
LD_PRELOAD).

>> No.  glibc 2.34 will behave as if an implicit -lpthread is present on
>> the linker program line.
>
> Good. This means a bullet-proof way for a distro to avoid the problem
> is to "rebuild the world" after importing glibc 2.34.

Yeah, but that's not good enough.  So I spent today on coming up with a
workaround in glibc.

>> No, it's unrelated.  The crash or other undefined behavior is a
>> consequence of actions of the link editor and cannot be reverted at run
>> time.
>
> In other words, the problem is that
>   - there are some/many binaries out there, that were produced by an 'ld'
> that did not anticipate the changes in glibc 2.34, and
>   - these binaries have a problem not when run directly, but only when
> run with LD_PRELOAD.
>
> Right?

No, glibc 2.34 won't need LD_PRELOAD to expose the bug.  LD_PRELOAD is
just a development aid that reveals the problem with glibc 2.33 and
earlier.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Bruno Haible:

> I'm confused again. Are you saying that the crash will occur when old
> binaries are being used with glibc 2.34 also *without* LD_PRELOAD?

Yes, that's how we discovered it.  And really is a problem.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* H. J. Lu via Libc-alpha:

> Alan extended the fix to PPC:
>
> commit 954b63d4c8645f86e40c7ef6c6d60acd2bf019de
> Author: Alan Modra 
> Date:   Wed Apr 19 01:26:57 2017 +0930
>
> Implement -z dynamic-undefined-weak
>
> -z nodynamic-undefined-weak is only implemented for x86.  (The sparc
> backend has some support code but doesn't enable the option by
> including ld/emulparams/dynamic_undefined_weak.sh, and since the
> support looks like it may be broken I haven't enabled it.)  This patch
> adds the complementary -z dynamic-undefined-weak, extends both options
> to affect building of shared libraries as well as executables, and
> adds support for the option on powerpc.

I'm not sure if this option is compatible with all compilers on POWER.

The old binutils behavior allowed the compiler to optimize this common
pattern

void f1 (void) __attribute__ ((weak));

void
f2 (void)
{
  if (f1 != 0)
f1 ();
}

into an unconditional call to f1 because the linker would replace the
call with a NOP if the symbol is undefined, reproducing the effect of
the if condition.  However, GCC 10 does not appear to perform this
optimization on powerpc64le.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Bruno Haible:

> You write:
>> Dynamic linking with weak symbols is not very well-defined.  ...
>> the code will crash if pthread_mutexattr_gettype is ever defined.
>
> In which situations will it crash?
>
>   (a) when the code is in an executable, that gets linked with '-lpthread'
>   and that does not use dlopen()?

The pthread_mutexattr_gettype is defined, but also pthread_once and the
weak symbols, so there is no problem because the link editor doesn't do
funny things.

>   (b) when the code is in an executable, that gets linked WITHOUT
>   '-lpthread' and that does not use dlopen()?

Yes, it will crash or behave incorrectly on most architectures *if*
pthread_mutexattr_gettype becomes available for some reason.

>   (c) when the code is in an executable, that gets linked WITHOUT
>   '-lpthread' and that does a dlopen("libpthread.so.X")?

This will probably work because pthread_mutexattr_gettype is not rebound
to the definition.

> Under which conditions will it crash?
>
>   ($) when the executable was built before glibc 2.34 and is run
>   with glibc 2.34 ?

Yes.

>   (%) when the executable is built against glibc 2.34 and is run
>   with glibc 2.34 ?

No.  glibc 2.34 will behave as if an implicit -lpthread is present on
the linker program line.

> And if it crashes, will setting the environment variable LD_DYNAMIC_WEAK [1]
> avoid the crash?

No, it's unrelated.  The crash or other undefined behavior is a
consequence of actions of the link editor and cannot be reverted at run
time.  The best we can do is to hide definitions of symbols like
pthread_mutexattr_gettype, therefore masking the existence of those
corrupted code paths (like glibc 2.33 and earlier do).

Thanks for looking into this.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Bruno Haible:

> Hi Florian,
>
>> Here's a fairly representative test case, I think.
>> 
>> #include 
>> #include 
>> 
>> extern __typeof (pthread_key_create) __pthread_key_create __attribute__ 
>> ((weak));
>> extern __typeof (pthread_once) pthread_once __attribute__ ((weak));
>> 
>> void
>> f1 (void)
>> {
>>   puts ("f1 called");
>> }
>> 
>> pthread_once_t once_var;
>> 
>> void __attribute__ ((weak))
>> f2 (void)
>> {
>>   if (__pthread_key_create != NULL)
>> pthread_once (&once_var, f1);
>> }
>> 
>> int
>> main (void)
>> {
>>   f2 ();
>> }
>> 
>> Building it with “gcc -O2 -fpie -pie” and linking with binutils 2.30
>> does not result in a crash with LD_PRELOAD=libpthread.so.0.
>
> Thank you for the test case. It helps the understanding.
>
> But I don't understand
>   - why anyone would redeclare 'pthread_once', when it's a standard POSIX
> function,

I could have used the weak pragma just like gnulib.

>   - why f2 is declared weak,

Oh, sorry for the confusion, it's a quick way to establish a full
compiler barrier with GCC.

>   - why the program skips its initializations in single-threaded mode,

It's a minimal test.

>   - why libpthread would be loaded through LD_PRELOAD or dlopen, given
> that the long-term statement has been that declaring a symbol weak
> has no effect on the dynamic linker [1][2][3][4]?

The relevant scenario here is LD_PRELOAD of a library that depends on
libpthread.so.0, so it's about indirect preloading of something that's
more usefull than just libpthread.so.0.  pthread_key_create would still
become available in this case.

>
> How about the following test case instead?
>
> =
> #include 
> #include 
>
> #pragma weak pthread_key_create
> #pragma weak pthread_once
>
> void
> do_init (void)
> {
>   puts ("initialization code");
> }
>
> pthread_once_t once_var;
>
> void
> init (void)
> {
>   if (pthread_key_create != NULL)
> {
>   puts ("multi-threaded initialization");
>   pthread_once (&once_var, do_init);
> }
>   else
> do_init ();
> }
>
> int
> main (void)
> {
>   init ();
> }
> =
>
> $ gcc -Wall -fpie -pie foo.c ; ./a.out 
> initialization code
>
> $ gcc -Wall -fpie -pie foo.c -Wl,--no-as-needed -lpthread ; ./a.out
> multi-threaded initialization
> initialization code
>
> What will change for this program with glibc 2.34?

If recompiled in this way: The presence of -lpthread will not matter, it
will always behave is if linked with -lpthread.

If not recompiled and linked without -lpthread against glibc 2.33 or
earlier, the behavior with the current glibc 2.34 snapshot is
architecture-dependent and also depends on the binutils version used for
linking the program.  In some cases, calling pthread_once jumps to
address zero (causing a crash), or the call to pthread_once is elided
from the executable.  This scenario can be emulated with older glibc
using LD_PRELOAD=libpthread.so.0.

I will try to come up with a way to preserve the glibc 2.33 behavior for
old binaries.  However, you should really remove those weak symbol
hacks.  They won't have any effect for glibc 2.34, and as explained,
they cause breakage with earlier glibc versions with certain
LD_PRELOAD-ed libraries.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Andreas Schwab:

> On Apr 27 2021, Florian Weimer via Binutils wrote:
>
>> I think we can provide an libBrokenGnulib.so preload module which
>> defines pthread_mutexattr_gettype to zero (as an absolute address), so
>> there is a kludge to keep old binaries working, but this is really
>> something that must be fixed in gnulib.
>
> It is likely that the use of weak pthread symbols is not confined to
> gnulib.

True.

Here's a fairly representative test case, I think.

#include 
#include 

extern __typeof (pthread_key_create) __pthread_key_create __attribute__ 
((weak));
extern __typeof (pthread_once) pthread_once __attribute__ ((weak));

void
f1 (void)
{
  puts ("f1 called");
}

pthread_once_t once_var;

void __attribute__ ((weak))
f2 (void)
{
  if (__pthread_key_create != NULL)
pthread_once (&once_var, f1);
}

int
main (void)
{
  f2 ();
}

Building it with “gcc -O2 -fpie -pie” and linking with binutils 2.30
does not result in a crash with LD_PRELOAD=libpthread.so.0.  That's
because the call-to-address-zero via pthread_once has been stubbed out
with NOPs, I think.  This is still not correct and will undoubtedly
cause problems because pthread_once is usually called for its side
effect.

With binutils 2.35, the call-to-address-zero is not stubbed out anymore,
but there is still no dynamic symbol reference for pthread_once, so
there is a crash when running with LD_PRELOAD=libpthread.so.0.

This looks like a pre-existing toolchain issue on POWER, related to the
thread I quoted at the start.  If we proceed with the glibc libpthread
changes as planned, things might turn out unacceptably bad.

I did a quick experiment and we cannot work around this using a
libpthread.so stub library and compatibility-only symbols in libc.so.6.
Unversioned weak symbols bind to the baseline symbol version, as they
probably should.  Even a __pthread_key_create@GLIBC_2.17 compatibility
symbol triggers this issue.  I have to think of something else to
salvage this.

Thanks,
Florian

Re: Undefined use of weak symbols in gnulib

[Florian Weimer]

* Paul Eggert:

> On 4/26/21 10:53 PM, Florian Weimer via Libc-alpha wrote:
>> This will become an urgent issue with glibc 2.34, which defines
>> pthread_mutexattr_gettype unconditionally.  Certain gnulib modules will
>> stop working until the binaries are relinked.
>
> Thanks for mentioning this. But in what sense will the modules stop
> working? I'm a little lost.
>
> glibc 2.34 also defines pthread_once unconditionally, right? So why
> doesn't code like this:
>
>   if (pthread_mutexattr_gettype != NULL)
> pthread_once (control, callback);
>
> continue to work in 2.34?

The link editor does not emitted a relocation referencing the
pthread_once symbol.  The function address in the GOT is always zero.
It does not matter if a pthread_once symbol exists at run time because
their is no reference to it.  Relinking the executable will of course
fix this and generate a reference to the pthread_once symbol, but
relinking is required.

For example, bison 3.7.4 built against glibc 2.33 has these pthread*
symbol references in its dynamic symbol table on powerpc64le-linux-gnu:

   58:   0 FUNCWEAK   DEFAULTUNDEF 
pthread_mutex_init@GLIBC_2.17 (2)
  117:   0 FUNCWEAK   DEFAULTUNDEF 
pthread_mutex_lock@GLIBC_2.17 (2)
  118:   0 NOTYPE  WEAK   DEFAULTUNDEF 
pthread_mutexattr_gettype
  120:   0 FUNCWEAK   DEFAULTUNDEF 
pthread_mutex_unlock@GLIBC_2.17 (2)

pthread_once is just not there.

Thanks,
Florian

Undefined use of weak symbols in gnulib

[Florian Weimer]

lib/glthread/lock.h has this:

| /* The way to test at runtime whether libpthread is present is to test
|whether a function pointer's value, such as &pthread_mutex_init, is
|non-NULL.  However, some versions of GCC have a bug through which, in
|PIC mode, &foo != NULL always evaluates to true if there is a direct
|call to foo(...) in the same function.  To avoid this, we test the
|address of a function in libpthread that we don't use.  */
| 
| #  pragma weak pthread_mutex_init
| #  pragma weak pthread_mutex_lock
| #  pragma weak pthread_mutex_unlock
| #  pragma weak pthread_mutex_destroy
| #  pragma weak pthread_rwlock_init
| #  pragma weak pthread_rwlock_rdlock
| #  pragma weak pthread_rwlock_wrlock
| #  pragma weak pthread_rwlock_unlock
| #  pragma weak pthread_rwlock_destroy
| #  pragma weak pthread_once
| […]

And:

| #  if !PTHREAD_IN_USE_DETECTION_HARD
| #   pragma weak pthread_mutexattr_gettype
| #   define pthread_in_use() \
|   (pthread_mutexattr_gettype != NULL || c11_threads_in_use ())
| #  endif

As far as I can tell gnulib uses this macro definition to implement
gl_once on glibc targets:

| #  define glthread_once(ONCE_CONTROL, INITFUNCTION) \
|  (pthread_in_use ()   
 \
|   ? pthread_once (ONCE_CONTROL, INITFUNCTION) 
 \
|   : (glthread_once_singlethreaded (ONCE_CONTROL) ? (INITFUNCTION (), 0) : 
0))

So the net effect is this:

  if (pthread_mutexattr_gettype != NULL)
pthread_once (control, callback);

Dynamic linking with weak symbols is not very well-defined.  On x86-64,
the link editor produces the expected dynamic symbol relocation for the
pthread_once call.  On other targets (notably POWER), no dynamic
relocation is produced, and the code will crash if
pthread_mutexattr_gettype is ever defined.

There is an old thread here covering related issues:

  Specify how undefined weak symbol should be resolved in executable
  

On glibc targets, there is another problem: weak references do not carry
symbol versions, so they can bind to base versions unexpectedly.

This will become an urgent issue with glibc 2.34, which defines
pthread_mutexattr_gettype unconditionally.  Certain gnulib modules will
stop working until the binaries are relinked.  I expect the issue is
already visible with earlier glibc versions if libpthread is
unexpectedly present at run time.

I think we can provide an libBrokenGnulib.so preload module which
defines pthread_mutexattr_gettype to zero (as an absolute address), so
there is a kludge to keep old binaries working, but this is really
something that must be fixed in gnulib.

Thanks,
Florian

Re: [PATCH 2/2] posix: Remove alloca usage for internal fnmatch implementation

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> -else if (*p == L_('|'))
> +else if (*p == L_(')') || *p == L_('|'))
>{
>  if (level == 0)
>{
> -NEW_PATTERN;
> -startp = p + 1;
> +size_t slen = opt == L_('?') || opt == L_('@')
> +   ? pattern_len : p - startp + 1;
> +CHAR *newp = malloc (slen * sizeof (CHAR));
> +if (newp != NULL)
> +  {
> +*((CHAR *) MEMPCPY (newp, startp, p - startp)) = L_('\0');
> +PASTE (PATTERN_PREFIX,_add) (&list, newp);
> +  }
> +if (newp == NULL || PASTE (PATTERN_PREFIX, _has_failed) (&list))
> +  {
> +retval = -2;
> +goto out;
> +  }
> +
> +if (*p == L_('|'))
> +  startp = p + 1;
>}

slen seems to be the wrong variable name.  But I don't know wh the
original code computes plen conditionally and then uses p - startp
unconditionally.  That seems wrong.  The discrepancy goes back to
821a6bb4360.  Do you see a case where the difference matters?

The == 0 checks for the recursive FCT calls are wrong because they treat
match failure the same as OOM and other errors (the -2 return value),
but that also is a pre-existing issue.

The conversation itself appears to be faithful.

Thanks,
Florian

Re: [PATCH 1/2] posix: Falling back to non wide mode in case of encoding error [BZ #14185]

[Florian Weimer]

* Florian Weimer via Libc-alpha:

> * Adhemerval Zanella via Libc-alpha:
>
>> +static int
>> +fnmatch_convert_to_wide (const char *str, struct scratch_buffer *buf,
>> + size_t *n)
>> +{
>> +  mbstate_t ps;
>> +  memset (&ps, '\0', sizeof (ps));
>> +
>> +  size_t nw = buf->length / sizeof (wchar_t);
>> +  *n = strnlen (str, nw - 1);
>> +  if (__glibc_likely (*n < nw))
>> +{
>> +  const char *p = str;
>> +  *n = mbsrtowcs (buf->data, &p, *n + 1, &ps);
>> +  if (__glibc_unlikely (*n == (size_t) -1))
>> +/* Something wrong.
>> +   XXX Do we have to set 'errno' to something which mbsrtows hasn't
>> +   already done?  */
>> +return -1;
>> +  if (p == NULL)
>> +return 0;
>> +  memset (&ps, '\0', sizeof (ps));
>> +}
>> +
>> +  *n = mbsrtowcs (NULL, &str, 0, &ps);
>> +  if (__glibc_unlikely (*n == (size_t) -1))
>> +return -1;
>> +  if (!scratch_buffer_set_array_size (buf, *n + 1, sizeof (wchar_t)))
>> +{
>> +  __set_errno (ENOMEM);
>> +  return -2;
>> +}
>> +  assert (mbsinit (&ps));
>> +  mbsrtowcs (buf->data, &str, *n + 1, &ps);
>> +  return 0;
>> +}
>>
>
> This, along with
>
>> +  if (r == -2 || r == 0)
>> +return r;
>
> below causes fnmatch to return the undocumented -2 error value, I think.
> Shouldn't we keep failing with -1 on error?

I see that this is what the existing code does.  So the patch looks okay
to me after all.

Thanks,
Florian

Re: [PATCH 1/2] posix: Falling back to non wide mode in case of encoding error [BZ #14185]

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> +static int
> +fnmatch_convert_to_wide (const char *str, struct scratch_buffer *buf,
> + size_t *n)
> +{
> +  mbstate_t ps;
> +  memset (&ps, '\0', sizeof (ps));
> +
> +  size_t nw = buf->length / sizeof (wchar_t);
> +  *n = strnlen (str, nw - 1);
> +  if (__glibc_likely (*n < nw))
> +{
> +  const char *p = str;
> +  *n = mbsrtowcs (buf->data, &p, *n + 1, &ps);
> +  if (__glibc_unlikely (*n == (size_t) -1))
> +/* Something wrong.
> +   XXX Do we have to set 'errno' to something which mbsrtows hasn't
> +   already done?  */
> +return -1;
> +  if (p == NULL)
> +return 0;
> +  memset (&ps, '\0', sizeof (ps));
> +}
> +
> +  *n = mbsrtowcs (NULL, &str, 0, &ps);
> +  if (__glibc_unlikely (*n == (size_t) -1))
> +return -1;
> +  if (!scratch_buffer_set_array_size (buf, *n + 1, sizeof (wchar_t)))
> +{
> +  __set_errno (ENOMEM);
> +  return -2;
> +}
> +  assert (mbsinit (&ps));
> +  mbsrtowcs (buf->data, &str, *n + 1, &ps);
> +  return 0;
> +}
>

This, along with

> +  if (r == -2 || r == 0)
> +return r;

below causes fnmatch to return the undocumented -2 error value, I think.
Shouldn't we keep failing with -1 on error?

Thanks,
Florian

Re: [PATCH 1/5] posix: Sync regex code with gnulib

[Florian Weimer]

* Adhemerval Zanella:

> On 19/01/2021 13:52, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
>>> Wouldn't be better to consolidate it on cdefs.h?
>> 
>> gnulib shouldn't be using cdefs.h on glibc systems, so I think a
>> separate header would be needed.
>
> What is the problem of using cdefs.h for internal implementation? 

Isn't regex_internal.h sync'ed with gnulib?

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/5] posix: Sync regex code with gnulib

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> Paul, this seemed to a common pattern scatter on multiple file in gnulib.
> Wouldn't be better to consolidate it on cdefs.h?

gnulib shouldn't be using cdefs.h on glibc systems, so I think a
separate header would be needed.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/2] posix: User scratch_buffer on fnmatch

[Florian Weimer]

* Bruno Haible:

> Paul Eggert asked:
>> > By the way, how important is it to support awful encodings like
>> > shift-JIS that contain bytes that look like '\'? If we don't have to 
>> > support these encodings any more, things get a bit easier.
>
> Here we are talking about locale encodings, and Shift_JIS (as well as
> SHIFT_JISX0213) are not usable as a locale encoding in glibc. See e.g.
> [1], [2].
>
> That's the reason why no Shift_JIS locale is listed in
> glibc/localedata/SUPPORTED. [3]

> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=3140
> [2] https://sourceware.org/legacy-ml/libc-alpha/2000-10/msg00311.html
> [3] https://sourceware.org/git/?p=glibc.git;a=blob;f=localedata/SUPPORTED

We used to have a fully supported product based on the original
Shift-JIS.  It did not require glibc changes (we package both localedef
and the locale sources, so it's easy to build custom locales), but other
GNU components had to be patched.

> Florian Weimer wrote:
>> There is a Shift-JIS variant which is ASCII-transparent (Windows-31J,
>> it's also specified by WhatWG/HTML5), so from a glibc point of view, it
>> would be just an ordinary charset like any other.
>> 
>> But feedback we have received is that the users who want Shift-JIS
>> really want the original thing.
>> 
>> We do not presently support either variant downstream, but one potential
>> way forward would be to turn Windows-31J into a fully supported glibc
>> charset with a corresponding ja_JP locale (which would imply downstream
>> support as well), and just hope that it displaces the original Shift-JIS
>> in the future.
>
> I don't think there's a real need for that. In the years 1995 ... 2005
> there was a lot of resistence against Unicode in Japan, because
> Unicode maps several slightly differently looking glyph images to the
> same glyph/character (even for Western encodings, for example the
> Polish accents look a bit different than the French ones), and - at
> the time - Unicode did not have means to disambiguate these, thus
> people complained about "characters are rendered incorrectly if you
> use Unicode". This has been resolved for more than 10 years already.

We saw commercial demand for Shift-JIS much later than that.  I think an
official Windows-31J-based ja_JP would still be welcomed at this point.

A Windows-31J locale could be added to localedata/SUPPORTED.  We have
not done that yet because someone wanted to look into alignment between
Windows, HTML/WhatWG and what we currently have in the source tree, but
that hasn't happened yet, unfortunately.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/2] posix: User scratch_buffer on fnmatch

[Florian Weimer]

* Paul Eggert:

> By the way, how important is it to support awful encodings like
> shift-JIS that contain bytes that look like '\'? If we don't have to 
> support these encodings any more, things get a bit easier. (Asking for
> a friend. :-)

There is a Shift-JIS variant which is ASCII-transparent (Windows-31J,
it's also specified by WhatWG/HTML5), so from a glibc point of view, it
would be just an ordinary charset like any other.

But feedback we have received is that the users who want Shift-JIS
really want the original thing.

We do not presently support either variant downstream, but one potential
way forward would be to turn Windows-31J into a fully supported glibc
charset with a corresponding ja_JP locale (which would imply downstream
support as well), and just hope that it displaces the original Shift-JIS
in the future.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: clang++ 11 compilation issues

[Florian Weimer]

* Alexandre Duret-Lutz:

> (1) lib/argmatch.h includes lib/gettext.h which fails as follows
>
>> clang++ -DHAVE_CONFIG_H -I. -I..  -I.. -I.. -I../buddy/src -I../lib -I../lib 
>>  -W -Wall -Werror -Wint-to-void-pointer-cast -Wzero-as-null-pointer-constant 
>> -Wcast-align -Wpointer-arith -Wwrite-strings -Wcast-qual -DXTSTRINGDEFINES 
>> -Wdocumentation -Wmissing-declarations -Woverloaded-virtual 
>> -Wmisleading-indentation -Wimplicit-fallthrough -Wnull-dereference 
>> -Wsuggest-override -Wpedantic -fvisibility=hidden 
>> -fvisibility-inlines-hidden -DSPOT_BUILD -std=c++17 -g -O -MT autcross.o -MD 
>> -MP -MF .deps/autcross.Tpo -c -o autcross.o autcross.cc
>> In file included from autcross.cc:34:
>> In file included from ../lib/argmatch.h:31:
>> ../lib/gettext.h:234:22: error: zero as null pointer constant 
>> [-Werror,-Wzero-as-null-pointer-constant]
>>   if (msg_ctxt_id != NULL)
>>  ^~~~
>>  nullptr
>> /usr/lib/llvm-11/lib/clang/11.0.1/include/stddef.h:84:18: note: expanded 
>> from macro 'NULL'
>> #define NULL __null
>>  ^

Would you be able to check whether __null is in the preprocessed
sources?  If it is there (and the lack of further logged expansions
suggests this), then this is a compiler bug.  __null is not zero, and
should be fine to use as a null pointer constant.  This is why NULL is
not defined as 0.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: rcs configure hang

[Florian Weimer]

* Kelly Wang:

> Hi Florian,
>
> Thanks for checking. Here is my system info:
> % uname -a
> Linux sjc-ads-7913 4.18.0-147.3.1.el8_1.x86_64 #1 SMP Wed Nov 27 01:11:44 UTC 
> 2019 x86_64 unknown unknown GNU/Linux
> % rpm -q kernel
> kernel-4.18.0-80.el8.x86_64
> kernel-4.18.0-147.3.1.el8_1.x86_64
>
> This is virtualization system.

What's the hypervisor?

(It may also be worthwhile to try with a more recent .el8 kernel.)

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 1/2] posix: User scratch_buffer on fnmatch

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> It removes the alloca usage on the string convertion to wide characters
> before calling the internal function.

We have a downstream-only patch to fall back the single byte handling in
case of multibyte decoding failure.  Basically it's a quick hack to fix
this bug:

  

Is this something we should upstream?  Or rework fnmatch so that * is
matched properly against arbitrary bytes?

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH v3 5/6] support: Add support_small_thread_stack_size

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> It returns the minimum stack size large enough to cover most internal
> glibc stack usage.

Looks okay to me.

Re: [PATCH 2/5] Import idx.h from gnulib

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> diff --git a/include/idx.h b/include/idx.h
> new file mode 100644
> index 00..ad7d31a2bc
> --- /dev/null
> +++ b/include/idx.h
> @@ -0,0 +1,113 @@
> +/* A type for indices and sizes.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 2, or (at your option)
> +   any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, see .  */

This file is linked into the library itself, so we can only accept
something that is under a compatible license (probably LGPLv2 or later).

Re: rcs configure hang

[Florian Weimer]

* Kelly Wang:

> You are right, after remove confdir3, rerun strace hang.
> Checked tr output, it stopped at bunch of mkdir and chdir and no further 
> steps after that.
> mkdir("confdir3", 0700) = 0
> chdir("confdir3")   = 0

Would you be able to share details of the file system used (XFS or
something else?) and the kernel version (uname -a, rpm -q kernel)?

Do you use virtualization or containers?

I would like to see if I can reproduce it internally.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Undefined behavior in lib/canonicalize-lgpl.c

[Florian Weimer]

We have received a report that the glibc realpath implementation
exhibits undefined behavior:

  

In gnulib, the code is in lib/canonicalize-lgpl.c:

234   if (!ISSLASH (dest[-1]))
235 *dest++ = '/';
236 
237   if (dest + (end - start) >= rpath_limit)
238 {
239   ptrdiff_t dest_offset = dest - rpath;
240   char *new_rpath;

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Use-after-free in test-perror2, test-strerror_r

[Florian Weimer]

The problem is visible with glibc 2.32 under valgrind:

==20== Invalid read of size 1
==20==at 0x483DAB4: strcmp (vg_replace_strmem.c:847)
==20==by 0x109414: main (test-perror2.c:84)
==20==  Address 0x4a1a3d0 is 0 bytes inside a block of size 17 free'd
==20==at 0x483A9F5: free (vg_replace_malloc.c:538)
==20==by 0x48E2134: strerror_l (in /usr/lib64/libc-2.32.so)
==20==by 0x109328: main (test-perror2.c:72)
==20==  Block was alloc'd at
==20==at 0x4839809: malloc (vg_replace_malloc.c:307)
==20==by 0x48CA03F: __vasprintf_internal (in /usr/lib64/libc-2.32.so)
==20==by 0x48A46F9: asprintf (in /usr/lib64/libc-2.32.so)
==20==by 0x48E2184: strerror_l (in /usr/lib64/libc-2.32.so)
==20==by 0x1092E2: main (test-perror2.c:67)
==20== 
==20== Invalid read of size 1
==20==at 0x483DAC8: strcmp (vg_replace_strmem.c:847)
==20==by 0x109414: main (test-perror2.c:84)
==20==  Address 0x4a1a3d1 is 1 bytes inside a block of size 17 free'd
==20==at 0x483A9F5: free (vg_replace_malloc.c:538)
==20==by 0x48E2134: strerror_l (in /usr/lib64/libc-2.32.so)
==20==by 0x109328: main (test-perror2.c:72)
==20==  Block was alloc'd at
==20==at 0x4839809: malloc (vg_replace_malloc.c:307)
==20==by 0x48CA03F: __vasprintf_internal (in /usr/lib64/libc-2.32.so)
==20==by 0x48A46F9: asprintf (in /usr/lib64/libc-2.32.so)
==20==by 0x48E2184: strerror_l (in /usr/lib64/libc-2.32.so)
==20==by 0x1092E2: main (test-perror2.c:67)

I think it's the test that's invalid.

This was reported as an actual grep test failure (without valgrind) on
32-bit Arm, where glibc malloc happens to return a different buffer
address for the internal allocation (so that msg3 != msg4).

test-strerror_r has the same issue.

Thanks,
Florian

Re: improve clang support (35)

[Florian Weimer]

* Bruno Haible:

> clang (at least in version >= 4), in C++ mode, supports the 'throw
> ()' declaration on functions, and uses it to optimize try/catch
> statements at the caller site.

I think throw() has been removed from C++20:
  
  
  

So it will soon break again.

Re: /*unsigned*/ int level3 in gen-uni-tables.c

[Florian Weimer]

* Bruno Haible:

>> This kind of narrowing initialization is no longer valid C++.
>
> Indeed, GCC 10.2.0 and clang give errors about this code when uses as C++ 
> code:
>
> $ gcc -Wall -O -S -x c++ foo.c
> foo.c:156:1: error: narrowing conversion of '4294967295' from 'unsigned int' 
> to 'int' [-Wnarrowing]
>   156 | };
>   | ^
> foo.c:156:1: error: narrowing conversion of '2147483648' from 'unsigned int' 
> to 'int' [-Wnarrowing]
> foo.c:156:1: error: narrowing conversion of '4294967295' from 'unsigned int' 
> to 'int' [-Wnarrowing]
>
> But why would this matter? This is C code, not C++ code.

Since gnulib is a copylib, it doesn't have much control over how it is
built.  It's one of the issues that came up while trying to build
things with a C++ compiler instead of a C compiler.

/*unsigned*/ int level3 in gen-uni-tables.c

[Florian Weimer]

gen-uni-tables.c produces types like this:

struct
  {
int header[1];
int level1[2];
short level2[2 << 7];
/*unsigned*/ int level3[16 << 4];
  }

Why is the unsigned commented out?  Some of the constants are so large
that they are treated as unsigned ints.

This kind of narrowing initialization is no longer valid C++.

Re: [musl] Building Bison 3.7 with musl (was Re: portability issues with unicodeio)

[Florian Weimer]

* Bruno Haible:

> Yes and no. The code is not making assumptions about a particular iconv()
> implementation. But it needs to distinguish two categories of replacements
> done by iconv():
>   - those that are harmless (for example when replacing a Unicode TAG
> character U+E00xx with an empty output),
>   - those that are better not presented to the user, if the programmer has
> specified a fallback (for example, replacing all non-ASCII characters
> with NUL, '?', or '*').
>
> The standards don't help in making the distinction.
>
> Therefore whether you consider said glibc and libiconv behaviour as
> "non-conforming" or not is irrelevant.

Could you sketch briefly what you need?  We have identified some issues
with the existing iconv interface.  If we add an enhancement, it would
make sense to cover these requirements.

Thanks,
Florian

Re: Missing '_Noreturn'

[Florian Weimer]

* Paul Eggert:

> On 7/26/20 3:10 PM, Gisle Vanem wrote:
>> Here's an error though:
>>
>>    test-dfa-match-aux.c
>>    test-dfa-match-aux.c(39): error C2381: 'dfawarn': redefinition;
>>    '__declspec(noreturn)' or '[[noreturn]]' differs
>>    lib\dfa.h(125): note: see declaration of 'dfawarn'
>
> That's a bug in MSVC. _Noreturn and 'inline' are not part of a
> function's type, so the definition of a function can say '_Noreturn'
> even though its declaration does not. (Admittedly it's bad style.)

The compiler is correct for [[noreturn]]:

| The first declaration of a function shall specify the noreturn
| attribute if any declaration of that function specifies the noreturn
| attribute.



I agree that the test needs fixing.

Thanks,
Florian

Re: Module with preprocessor utilities

[Florian Weimer]

* Marc Nieper-Wißkirchen:

> Am Fr., 24. Juli 2020 um 10:05 Uhr schrieb Florian Weimer 
> :
>
>> It's still a candidate for an RFE.  Martin Sebor has been working on
>> such warnings.  I'm going to talk to him.
>
> It may also be useful for optimizations because the compiler may make
> assumptions.
>
>> >> It's also undefined when you pass the address of something that is not
>> >> the first element of an array of type double to foo1.  (Undefined in the
>> >> sense that the standard does not say what happens in that case.)
>> >
>> > Yes, but a pointer to (an existing) double fulfills this requirement.
>>
>> I don't think that's how the standard defines “first element of an
>> array”.  I suspect the intent was that it the pointer has to point to an
>> array of at least the indicated number of elements, but that's not what
>> the standard says.
>
> According to ISO C, "an array type describes a contiguously allocated
> nonempty set of objects with a particular member object type, called
> the element type". Thus, given
>
> double x;
> double *p = &x;
>
> the pointer p points to the first element of an array (of length 1).

This is the case that is unclear:

double x[2];
double *p = &x[1];

The standard explicitly says “first element of an array”. 

Thanks,
Florian

Re: Module with preprocessor utilities

[Florian Weimer]

* Marc Nieper-Wißkirchen:

> Am Fr., 24. Juli 2020 um 08:53 Uhr schrieb Florian Weimer 
> :
>
>> * Bruno Haible:
>
>> > (This is with gcc 10.1.0.)
>> >
>> > clang, OTOH, produces warnings for both foo1 and foo2.
>> >
>> > But I won't spend time to report a GCC bug on this, because - as you said -
>> > without the ability to declare a pointer to an incomplete type or a 'void 
>> > *'
>> > as non-null, this language feature is worthless.
>
> I don't think that it is a bug in the technical sense. A C99 compiler
> does not have to warn as, in general, it is undecidable whether a
> particular argument is NULL or not.

It's still a candidate for an RFE.  Martin Sebor has been working on
such warnings.  I'm going to talk to him.

>> It's also undefined when you pass the address of something that is not
>> the first element of an array of type double to foo1.  (Undefined in the
>> sense that the standard does not say what happens in that case.)
>
> Yes, but a pointer to (an existing) double fulfills this requirement.

I don't think that's how the standard defines “first element of an
array”.  I suspect the intent was that it the pointer has to point to an
array of at least the indicated number of elements, but that's not what
the standard says.

Thanks,
Florian

Re: Module with preprocessor utilities

[Florian Weimer]

* Bruno Haible:

> Also, if that's the intent of the syntax, it has been overlooked by the
> GCC developers. See:
>
>  decl.c 
> #include 
>
> extern void foo1 (double x [static 1]);
> extern void foo2 (double x []) __attribute__ ((__nonnull__(1)));
>
> int bar ()
> {
>   foo1 (NULL);
>   foo2 (NULL);
>   return 1;
> }
> =
> $ gcc -Wall -S decl.c
> decl.c: In function 'bar':
> decl.c:9:3: warning: null argument where non-null required (argument 1) 
> [-Wnonnull]
> 9 |   foo2 (NULL);
>   |   ^~~~
>
> (This is with gcc 10.1.0.)
>
> clang, OTOH, produces warnings for both foo1 and foo2.
>
> But I won't spend time to report a GCC bug on this, because - as you said -
> without the ability to declare a pointer to an incomplete type or a 'void *'
> as non-null, this language feature is worthless.

It's also undefined when you pass the address of something that is not
the first element of an array of type double to foo1.  (Undefined in the
sense that the standard does not say what happens in that case.)

Thanks,
Florian

Re: Optimize three-valued comparison between integers

[Florian Weimer]

* Bruno Haible:

> A comment in Luca Saiu 'jitter' project [1] brought my attention to 3-valued
> comparison and the book "Hacker's Delight".
>
> ===
> int sign1 (long n1, long n2)
> {
>   return (n1 > n2 ? 1 : n1 < n2 ? -1 : 0);
> }
>
> int sign2 (long n1, long n2)
> {
>   return (n1 < n2 ? -1 : n1 > n2);
> }
>
> int sign3 (long n1, long n2)
> {
>   return (n1 > n2) - (n1 < n2);
> }
> ===
>
> Which of these, do you think, generates better code? The important fact,
> nowadays, is that conditional jumps cause the CPU to stall when it has
> mispredicted the jump. So, 1 or 2 more or less ALU instructions are
> irrelevant, but conditional jumps are not.
>
> You find conditional jumps in code (x86) produced by GCC versions
>
> sign1: 4.1.2 4.2.4 4.3.6 4.4.7 4.5.4 4.6.4 4.7.3 4.8.5 4.9.4 5.5.0 7.5.0 
> 8.4.0 9.3.0 10.1.0
> sign2: 4.1.2 4.2.4 4.3.6 4.4.7 7.5.0 8.4.0 9.3.0
> sign3: 
>
> So, the third variant comes out best.

On aarch64, sign1 and sign2 are one instruction shorter, and do not
contain branches, either.  On s390x, all three variants use conditional
branches, but the first one is the shortest.

There is also this:

int sign4 (long n1, long n2)
{
  return (char) ((n1 > n2) - (n1 < n2));
}

It's one instruction shorter on x86-64 than sign3, but it's worse on
other architectures.  (One of the x86-64 quirks is that conditional sets
do not affect the full register, only a byte.)

Thanks,
Florian

Re: new module 'aligned-malloc'

[Florian Weimer]

* Paul Eggert:

> On 7/23/20 5:24 AM, Florian Weimer wrote:
>> Ah, 4096 is too small.
>>
>> For large allocations (above the mmap threshold), what happens in the
>> example is that the process VMA limit is eventually exhausted, at which
>> malloc falls back to sbrk, and then the algorithm succeeds.  I don't
>> think it's a good idea to trigger ENOMEM, it's certainly not
>> thread-safe.
>
> I'm not sure I see what ENOMEM has to do with it if the algorithm is
> succeeding.

During the mmap phase, the algorithm never achieves alignment.  Once
mmap fails with ENOMEM (likely because the number of VMAs would exceed
the per-process limit), malloc switch to the brk heap, where the
algorithm eventually produces an aligned allocation.  Only then the loop
terminates.

Thanks,
Florian

Re: new module 'aligned-malloc'

[Florian Weimer]

* Paul Eggert:

> On 7/22/20 12:13 AM, Florian Weimer wrote:
>> I don't think it will work.  Try to get an allocation of 4096 bytes with
>> 4096 bytes alignment using glibc malloc this way.
>
> That's just a mental exercise since glibc malloc already has
> aligned_alloc, but I took the challenge anyway and found that it's
> eminently doable with glibc malloc alone. Run the attached program,
> and it should exit with status 0. At least, it works for me.

Ah, 4096 is too small.

For large allocations (above the mmap threshold), what happens in the
example is that the process VMA limit is eventually exhausted, at which
malloc falls back to sbrk, and then the algorithm succeeds.  I don't
think it's a good idea to trigger ENOMEM, it's certainly not
thread-safe.

Thanks,
Florian

Re: new module 'aligned-malloc'

[Florian Weimer]

* Paul Eggert:

> On 7/21/20 8:51 AM, Florian Weimer wrote:
>> The official aligned_alloc produces pointers compatible with free.
>> This module cannot do that.
>
> I don't see why not, at least on platforms of interest to Gnulib. On
> systems that provide no native way to do an aligned allocation, we
> merely keep calling malloc with suitable arguments until we get a
> pointer that is suitably aligned. We then free all the unsuitable
> storage we allocated along the way, and return the good pointer.

I don't think it will work.  Try to get an allocation of 4096 bytes with
4096 bytes alignment using glibc malloc this way.

Thanks,
Florian

Re: new module 'aligned-malloc'

[Florian Weimer]

* Paul Eggert:

> Also, how about naming the new module 'aligned-alloc' and having it
> implement aligned_alloc? That would make for more-seamless
> integration.

The official aligned_alloc produces pointers compatible with free.
This module cannot do that.

Thanks,
Florian

Re: Relicence explicit_bzero to GPL2+/LGPL3 dual

[Florian Weimer]

* Bastien ROUCARIES:

> Could be possible to relicence this module under dual GPL2+ LGPL3 licence?
>
> Code is from glibc thus LGPL2+ and it will be useful for some program
> like fwknop

Isn't this implied by LGPL2+?

Thanks,
Florian

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Paul Eggert:

> On 3/10/20 12:30 PM, Florian Weimer wrote:
>> The glibc implementation needs /proc to avoid the race.  There is no
>> way around that, otherwise we introduce a security vulnerability.
>
> It is unfortunate that we have dueling paranoia here. coreutils mknod is 
> paranoid so it uses lchmod to avoid a race, and then glibc lchmod is paranoid 
> so 
> it refuses to work with lchmod unless /proc is mounted.

I now wonder if neither gnulib nor glibc should pretend that they can
implement lchmod and fchmodat on Linux in a usable fashion.

I added the emulation to glibc mostly because it was in gnulib.
Otherwise, I would have insisted that a proper system call be
implemented first.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Paul Eggert:

> On 3/10/20 12:30 PM, Florian Weimer wrote:
>
>> Is the concern here that
>> you would get a different mode from the requested one if you use
>> umask+mknod and not mknod+some form of chmod?
>
> Yes. See:
>
> https://bugs.gnu.org/14371

Wrong bug?  There is no --mode argument involved in that bug report,
as far as I can see.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Pádraig Brady:

> On 10/03/2020 11:52, Florian Weimer wrote:
>> * Pádraig Brady:
>> 
>>> On 09/03/2020 18:51, Paul Eggert wrote:
>>>> On 3/9/20 10:30 AM, Pádraig Brady wrote:
>>>>
>>>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
>>>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
>>>>> rawhide in a chroot.
>>>>> https://bugzilla.redhat.com/1811038
>>>>
>>>> I don't understand that bug report. The strace diff you mentioned in
>>>> Comment 4 looks like the new mknod command is working. And yet the
>>>> original bug report says new mknod command is complaining "Operation not
>>>> supported".
>>>>
>>>> Is the problem that some filesystems don't work with the chmod
>>>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
>>>> but not for Mohan (the original bug report)?
>>>
>>> Right, the strace is from my working mknod(1)
>>> to show the differences between 8.31 and 8.32.
>>>
>>> I've requested an strace from the failing system.
>> 
>> I guess it's possible that just isn't mounted at this point.
>> 
>> The glibc implementation will definitely *not* add racy fallback in
>> case /proc is not available, so this will not work until someone
>> implements the required system call.
>> 
>> It's not clear to my why the mknod command would need fchmodat at all.
>> With the -m argument, it should simply set the umask to 0, and pass
>> the mode bits to the mknod function.
>
> umask is not used so as to cater for discrepancies between process and 
> default ACL masks:
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=v8.21-51-ge7198a67b

I just don't understand this explanation.  Is the concern here that
you would get a different mode from the requested one if you use
umask+mknod and not mknod+some form of chmod?

> An update re this issue.
> The strace was supplied in https://bugzilla.redhat.com/1811038
> which shows there is no fallback to chmod() in lchmod().
> Now the gnulib code does fallback so this issue must be in the glibc 
> implementation.

The glibc implementation needs /proc to avoid the race.  There is no
way around that, otherwise we introduce a security vulnerability.

Re: [PATCH] fchmodat, lchmod: port to buggy Linux filesystems

[Florian Weimer]

* Pádraig Brady:

> On 09/03/2020 18:51, Paul Eggert wrote:
>> On 3/9/20 10:30 AM, Pádraig Brady wrote:
>> 
>>> A very similar "ENOTSUP" problem is being reported with coreutils-8.32
>>> with `mknod -m 666 /dev/random c 1 8` when trying to build Fedora
>>> rawhide in a chroot.
>>> https://bugzilla.redhat.com/1811038
>> 
>> I don't understand that bug report. The strace diff you mentioned in
>> Comment 4 looks like the new mknod command is working. And yet the
>> original bug report says new mknod command is complaining "Operation not
>> supported".
>> 
>> Is the problem that some filesystems don't work with the chmod
>> /proc/self/fd/NNN trick, and that it worked for you (the strace diff)
>> but not for Mohan (the original bug report)?
>
> Right, the strace is from my working mknod(1)
> to show the differences between 8.31 and 8.32.
>
> I've requested an strace from the failing system.

I guess it's possible that just isn't mounted at this point.

The glibc implementation will definitely *not* add racy fallback in
case /proc is not available, so this will not work until someone
implements the required system call.

It's not clear to my why the mknod command would need fchmodat at all.
With the -m argument, it should simply set the umask to 0, and pass
the mode bits to the mknod function.

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Florian Weimer]

* Rich Felker:

> Note that in any case, musl's lchmod/fchmodat is not affected since it
> always refuses to change symlink modes; I did this because I was
> worried that chmod on the magic symlink in /proc might pass through
> not just to the symlink it refers to, but to the symlink target if one
> exists. With current kernel versions it seems that does not happen; is
> it safe to assume it doesn't?

I saw it happen with sshfs over FUSE. 8-/

Yet another reason to put in a check before performing the chmod.

> Further, I've found some inconsistent behavior with ext4: chmod on the
> magic symlink fails with EOPNOTSUPP as in Florian's test, but fchmod
> on the O_PATH fd succeeds and changes the symlink mode. This is with
> 5.4. Cany anyone else confirm this? Is it a problem?

Interesting. Let me update the other thread.

Re: bug#39236: [musl] coreutils cp mishandles error return from lchmod

[Florian Weimer]

* Paul Eggert:

> On 1/22/20 2:05 PM, Rich Felker wrote:
>> I think we're approaching a consensus that glibc should fix this too,
>> so then it would just be gnulib matching the fix.
>
> I installed the attached patch to Gnulib in preparation for the upcoming 
> glibc fix. The patch causes fchmodat with AT_SYMLINK_NOFOLLOW to work on 
> non-symlinks, and similarly for lchmod on non-symlinks. The idea is to 
> avoid this sort of problem in the future, and to let Coreutils etc. work 
> on older platforms as if glibc 2.32 (or whatever) is already in place.

The lchmod implementation based on /proc tickles an XFS bug:

  

Re: GNUlib unicode encoding causes smart quotes to be displayed in program's output

[Florian Weimer]

* Tim Rühsen:

> On 12/5/19 4:12 PM, Wes Hurd wrote:
>> Hi,
>> 
>> It seems GNUlib quote encoding goes to Unicode smart quotes, which causes
>> command-line program output to be in smart quotes.
>> Smart quotes are dangerous for programmers and technical users, and should
>> be avoided in program output.
>> 
>> Originally noticed with wget -
>> https://savannah.gnu.org/bugs/index.php?57356#comment1
>> 
>> Can you change it to use only regular typed " quotes , at least with STDOUT
>> / STDERR ?
>
> All the GNU tools support localization. And there seems to be some kind
> of inconsistency.
>
>
> $ LANGUAGE=de cp
> cp: Fehlender Dateioperand
> „cp --help“ liefert weitere Informationen.
>
>
> $ LANGUAGE=de wget
> wget: URL fehlt
> Aufruf: wget [OPTION]... [URL] …
>
> »wget --help« gibt weitere Informationen.
>
>
> Which one is correct ?

Both are, for Germany and Austria.  »« quotes are in Latin-1, which once
was a decisive advantage.  On the other hand, »« used to be restricted
to professional typesetting.

In Switzerland, «» are used instead of »« quotes, if I recall correctly.

Thanks,
Florian

[PATCH] wctob: mbtowc is declared in

[Florian Weimer]

* m4/wctob.m4 (gl_FUNC_WCTOB): Include .
---
 m4/wctob.m4 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/m4/wctob.m4 b/m4/wctob.m4
index 05c1237ee..90de3e8ad 100644
--- a/m4/wctob.m4
+++ b/m4/wctob.m4
@@ -81,6 +81,7 @@ int main ()
included before .  */
 #include 
 #include 
+#include 
 #include 
 #include 
 int main ()
-- 
2.21.0

Re: accept4 and SOCK_NONBLOCK

[Florian Weimer]

* Richard W. M. Jones:

> On Tue, Aug 20, 2019 at 05:37:58PM +0200, Florian Weimer wrote:
>> * Richard W. M. Jones:
>> 
>> > As for why accept4 is being replaced at all, the only reference to it
>> > in config.log is:
>> >
>> >   configure:25630: checking whether accept4 is declared
>> >   configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security 
>> > -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions 
>> > -fstack-protector-strong -grecord-gcc-switches 
>> > -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 
>> > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic 
>> > -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  
>> > conftest.c >&5
>> >   configure:25630: $? = 0
>> >   configure:25630: result: yes
>> 
>> What does the test program look like?  Does it include ?
>> accept4 requires _GNU_SOURCE.
>
> I'm not sure how to find out.  However m4/accept.m4 (supplied
> by gnulib itself) does contain:
>
>   dnl Persuade glibc  to declare accept4().
>   AC_REQUIRE([AC_USE_SYSTEM_EXTENSIONS])
>
> so I guess the test *ought* to be using _GNU_SOURCE.

And the configure test acctually succeeded, which I missed.  So it has
to be something else.  You'll have to investigate the generated files to
see what's going wrong there.

Thanks,
Florian

Re: accept4 and SOCK_NONBLOCK

[Florian Weimer]

* Richard W. M. Jones:

> As for why accept4 is being replaced at all, the only reference to it
> in config.log is:
>
>   configure:25630: checking whether accept4 is declared
>   configure:25630: gcc -c -O2 -g -pipe -Wall -Werror=format-security 
> -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions 
> -fstack-protector-strong -grecord-gcc-switches 
> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 
> -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic 
> -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection  
> conftest.c >&5
>   configure:25630: $? = 0
>   configure:25630: result: yes

What does the test program look like?  Does it include ?
accept4 requires _GNU_SOURCE.

Thanks,
Florian

Re: [PATCH 1/2] copy-file-range: new module

[Florian Weimer]

* Bruno Haible:

> Pádraig Brady wrote:
>> https://sourceware.org/ml/libc-alpha/2019-06/msg00873.html
>
> In the NEWS entry, Florian writes:
>
>Applications which use the
>copy_file_range function will have to be run on kernels which implement
>the copy_file_range system call.
>
> I find this misleading. Coreutils will want to continue to use
> copy_file_range whenever the libc provides this function, and at the
> same time be able to run on all Linux kernel versions.

What's misleading about it?  If you want copy_file_range functionality,
you will need an implementation of it, and since glibc no longer
contains one after this change, you need one from the kernel.

You can still call the function, of course, but it's use is really
limited because it sets errno ENOSYS and returns -1.

Thanks,
Florian

Re: [PATCH 1/2] copy-file-range: new module

[Florian Weimer]

* Paul Eggert:

> +/* Emulation of copy_file_range.
> +   Copyright 2017-2019 Free Software Foundation, Inc.
> +
> +   This program is free software: you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see .  */
> +
> +/* This file is adapted from glibc io/copy_file_range-compat.c, with a
> +   small number of changes to port to non-glibc platforms.  */

This is not a valid implementation of copy_file_range anymore.  Please
see the discussion here:

  

If you ship this in gnulib, you should at least call this function by a
different name.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Bruno Haible:

> Florian Weimer wrote in
> <https://lists.gnu.org/archive/html/bug-gnulib/2019-05/msg00087.html>:
>> The relevant case is where there is no error, and we do not call _exit.
>> I'm worried that the current implementation introduces a use-after-free
>> bug under certain, quite reasonable circumstances.  All that is needed
>> is a shared object that tries to log something to stderr from an ELF
>> destructor.  I don't think that's something that can be ruled out, or
>> can be assumed to happen in development environments only.
>
> OK, now you have described the problem in an understandable way.
>
> Let me rephrase the dilemma:
>
>   1) POSIX guarantees that we can detect write errors [up to the file
>  system layer of the kernel - I'm not worried about I/O errors on
>  the actual device] through fclose(), and kernels implement this.
>  Neither POSIX nor Linux guarantees that we can detect write errors
>  without calling fclose().
>
>   2) POSIX says "After the call to fclose(), any use of stream results in
>  undefined behavior." [1]
>
> So, we need to call fclose(stderr) at a moment when we know that stderr
> will not be used any more.
>
> We have
>   * applications (like the coreutils programs), and
>   * environments which can modify the behaviour of these applications,
> like shared objects added through LD_PRELOAD, or sanitizers [2].

This doesn't have to do anything with LD_PRELOAD.  It really depends
what kind of ELF destructors and atexit handlers are present in the
process image.

> The solution I would propose is to
>   - By default, assume that the environment does not modify the behaviour
> of the application. That is, that the application executes its code
> and nothing more.
>   - Let the environment tell the application (through an environment
> variable) that it is modifying its behaviour.
>
> For the first case, the current 'closeout' module is perfect.
>
> For the other case, we can introduce, next to the !SANITIZE_ADDRESS test,
> tests for
>   getenv ("LD_PRELOAD") != NULL
>   getenv ("ASAN_OPTIONS") != NULL
>   getenv ("TSAN_OPTIONS") != NULL
>   getenv ("MSAN_OPTIONS") != NULL
>   getenv ("LSAN_OPTIONS") != NULL
> We can add more such environment variables as needed. getenv() lookups
> don't make system calls; so they are cheap.

The application may have called clearenv before that.

What's so bad about closing the underyling file descriptor after
duplicating it?  It's much more portable than the other stdio hacks the
gnulib code contains today.  It will not have the desired effect on some
platforms, and on others, it is redundant because the write system call
performs ENOSPC checks, even on NFS.  It fixes a real problem our users
reported on Linux.  Why can't we make this work correctly out of the
box?

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Paul Eggert:

> On 5/13/19 12:00 AM, Florian Weimer wrote:
>>> I don't see any way around this problem in general with the closeout
>>> module's current API, because when it discovers an I/O error it calls
>>> _exit, and _exit also clashes with that kind of cleanup handling.
>> This isn't the problem.  fflush may also realistically cause SIGPIPE to
>> be delivered to the process.  That's all fine.
>
> It might be fine for some of these environments, but surely it can't
> be fine for others. If I'm relying on the startup routines to issue
> some sort of report for any non-signalling exit, then calls to _exit
> will bypass the report.

Sorry, I don't follow.  If fflush (stderr) fails (or terminates the
process with SIGPIPE), in the current code, then at least there isn't a
memory safety violation.  It's also a bit unlikely that code running
later could do anything useful, given that the process or the entire
system is in such a bad state.

As far as I understand, the close_stdout function intends to avoid
silent truncation due to an ENOSPC error.  It does not call fsync, so it
cannot prevent data loss as the result of a subsequent system crash, but
at least it prevents silent ENOSPC data corruption with the Linux NFS
implementation.

>> Look at the existing workaround
>> for sanitizers, and the comment in close_stream.  The code is buggy
>
> I agree the code is a hack. But it's not buggy: it's portable to any
> environment that conforms to POSIX, and that's a wide variety of
> environments.

As far as I know, POSIX does not say anywhere something like, “No
function in this volume of POSIX.1‐2008 shall write anything to the
standard error stream” or “No function in this volume of POSIX.1‐2008
shall call the perror function”.

> The problem seems to be that people want to run these applications in
> debugging environments that don't conform to POSIX. While I'm
> sympathetic to that goal, it's not a high-priority-enough situation to
> call the current code "buggy".

I don't think this issue is restricted to debugging environments.
Printing diagnostics to stderr is always a bit iffy, but can happen in
many cases.

> A better fix here would not be to pile yet another hack into this
> hacky module. It would be to write a better module (we could call it
> "flushout", say) that would define a function ("flushout_stream", say)
> that would act like fflush but would do a better job with NFS-based
> streams by playing the dup+close+fsync trick. We could then modify
> coreutils etc. to use this new module instead of the old one. The hard
> part is the "modify coreutils etc." part, because flushout_stream will
> be less convenient to use than the current API.

That's why I think we need to fix close_stdout instead.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Paul Eggert:

>> As long as you link libraries dynamically, any of the directly or indirectly
>> linked libraries can introduce an ELF destructor or atexit() handler anytime,
>
> I don't see any way around this problem in general with the closeout
> module's current API, because when it discovers an I/O error it calls
> _exit, and _exit also clashes with that kind of cleanup handling.

This isn't the problem.  fflush may also realistically cause SIGPIPE to
be delivered to the process.  That's all fine.

The relevant case is where there is no error, and we do not call _exit.
I'm worried that the current implementation introduces a use-after-free
bug under certain, quite reasonable circumstances.  All that is needed
is a shared object that tries to log something to stderr from an ELF
destructor.  I don't think that's something that can be ruled out, or
can be assumed to happen in development environments only.

Even if I didn't have a user bug report about this issue, I would have
expected the current behavior to be unacceptable from a
quality-of-implementation perspective.  Look at the existing workaround
for sanitizers, and the comment in close_stream.  The code is buggy and
needs to be fixed.

My offer to write patch along the lines that Neil Brown sketched still
stands, but so does my concern that polishing it will require more work
than the patch itself.

Thanks,
Florian

Re: Coverity false positives triggered by gnulib's implementation of base64

[Florian Weimer]

* Kamil Dudka:

>> For example, how do you know that the reports are false positives and not
>> true positives?
>
> I think it was obvious from my previous explanation:
>
> (1) You need to check (by manual review) that the source of data is really 
> untrusted.
>
> (2) You need to check (by manual review) that there is no sufficient check
> on the data.
>
> (3) You need to check (by manual review) that the sink function is really 
> vulnerable to data from untrusted source.
>
> When doing step (3), I verified that Gnulib's base64_encode() can safely 
> process data from untrusted source.  Then I wanted to record this information 
> into the source code so that other users of Gnulib do not need to verify this 
> each time they run Coverity on a project that bundles Gnulib's implementation 
> of base64_encode().

Does the annotation make the base64 functions trusted in the sense that
they now turn untrusted data into trusted data?  That would be
undesirable in my opinion.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Kamil Dudka:

> On Friday, May 10, 2019 12:17:11 AM CEST Paul Eggert wrote:
>> On 5/8/19 11:39 PM, Florian Weimer wrote:
>> > atexit handlers run before ELF destructors (and some C++ destructors).
>> > There can also be multiple such handlers.  So it's not true that an
>> > atexit handler always runs last.
>> 
>> OK, but this shouldn't be a problem with any applications currently
>> using close_stdout. At least, none of the applications I know about.
>
> How would you know if they did?
>
> As long as you link libraries dynamically, any of the directly or indirectly 
> linked libraries can introduce an ELF destructor or atexit() handler anytime, 
> which would take an immediate effect even on your already built applications.
> People also like to use instrumentation libraries enforced by LD_PRELOAD in 
> their test environment.  Those can easily clash with such cleanup handlers.

Right.  And I found close_stdout after someone reported problems with an
LD_PRELOAD-like mechanism and its logging output.  For some users, this
is an actual problem.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Bernhard Voelker:

> On 5/6/19 9:19 PM, Florian Weimer wrote:
>> How would a programmer check that close_stdout has run, to determine
>> that stdout and stderr are now invalid, to avoid the memory corruption?
>
> lib/closeout.c:98:
>   "Since close_stdout is commonly registered via 'atexit', [...]"
>
> close_stdout is used right before the process ends, so I don't see
> what further actions would follow.

atexit handlers run before ELF destructors (and some C++ destructors).
There can also be multiple such handlers.  So it's not true that an
atexit handler always runs last.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Paul Eggert:

> Florian Weimer wrote:
>>> You can achieve that "actual close call" using
>>>
>>>error = close(dup(fileno(stdout)));
>>>
>>> so you don't actually need to "fclose" if you don't want to.
>>> Any 'close' will do, it doesn't have to be the "last close".
>> Hah, thanks for this suggestion!  So something good came out of this
>> thread after all.  The big advantage of this approach is that this will
>> preserve the descriptor and the stream, so that further diagnostics from
>> the process are not suppressed.
>
> That trick won't work if the dup fails.

You can do an fsync in this case.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* NeilBrown:

> On Tue, May 07 2019, Bruno Haible wrote:
>
>> Assaf Gordon wrote:
>>> 4.
>>> "fflush" instead of "fclose" seems to work OK, but I do not know
>>> if there are other side effects:
>>> 
>>>$ ./aa stdout fflush > /dev/full && echo ok || echo error
>>>aa: fflush failed: No space left on device
>>>error
>>
>> Except that it does not work OK on NFS, as explained by the comment
>> in close-stream.c (written in 2006):
>>
>>Even calling fflush is not always sufficient,
>>since some file systems (NFS and CODA) buffer written/flushed data
>>until an actual close call.
>
> You can achieve that "actual close call" using
>
>   error = close(dup(fileno(stdout)));
>
> so you don't actually need to "fclose" if you don't want to.
> Any 'close' will do, it doesn't have to be the "last close".

Hah, thanks for this suggestion!  So something good came out of this
thread after all.  The big advantage of this approach is that this will
preserve the descriptor and the stream, so that further diagnostics from
the process are not suppressed.

Would someone who is familiar with the gnulib policies and procedures
please turn this into a proper patch, with error checking and all?  I
can try, but it's probably more work for you to bring what I could write
into an acceptable shape, than write the patch from scratch.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Bernhard Voelker:

> On 5/6/19 5:47 PM, Florian Weimer wrote:
>> * Bernhard Voelker:
>>> What is the problem?  I mean if it is use-after-free as mentioned in
>>> the first mail, then write() after fflush() without error checking via
>>> another fflush() is in the same category, isn't it?
>> 
>> No, there is no memory corruption involved because stdout and stderr
>> remain valid.
>
> IMO that's easier to detect than a write() without a following error
> checking; the consequences may also be quite fatal for the user.

I do not understand this comment.  Do you mean fwrite?

How would a programmer check that close_stdout has run, to determine
that stdout and stderr are now invalid, to avoid the memory corruption?

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Bernhard Voelker:

> On 5/6/19 2:05 PM, Florian Weimer wrote:
>>>> On 4/29/19 2:45 PM, Florian Weimer wrote:
>>>>> I get that error checking is important.  But why not just use ferror and
>>>>> fflush?  Closing the streams is excessive and tends to introduce
>>>>> use-after-free issues, as evidenced by the sanitizer workarounds.
>
>> This means that for Linux at least, close_stdout should just call
>> fflush, not fclose.
>
> What is the problem?  I mean if it is use-after-free as mentioned in
> the first mail, then write() after fflush() without error checking via
> another fflush() is in the same category, isn't it?

No, there is no memory corruption involved because stdout and stderr
remain valid.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Florian Weimer:

> * Eric Blake:
>
>> On 4/29/19 2:45 PM, Florian Weimer wrote:
>>> I get that error checking is important.  But why not just use ferror and
>>> fflush?  Closing the streams is excessive and tends to introduce
>>> use-after-free issues, as evidenced by the sanitizer workarounds.
>>
>> If I recall the explanation, at least some versions of NFS do not
>> actually flush on fflush(), but wait until close(). If you want to avoid
>> data loss and ensure that things written made it to the remote storage
>> while detecting every possible indication when an error may have
>> prevented that from working, then you have to go all the way through
>> close().
>
> Any file system on Linux does this to a varying degree (unlike Solaris
> and Windows, I think).  If you want to catch low-level I/O errors, you
> need to call fsync after fflush.  And I doubt this is something we want
> to do because it will result in bad-looking performance.
>
> But the NFS aspect is somewhat plausible at least.
>
> I can try to figure out if NFS makes a difference for Linux here,
> i.e. if there are cases where a write will succeed, but only an
> immediately following close will report an error condition that is
> known, in principle, even at the time of the write.  A difference
> between hard and soft NFS mounts could matter in this context.

Start of thread: <https://lists.gnu.org/r/bug-gnulib/2019-04/msg00059.html>

I've been told that on Linux, close does not report writeback errors.
So the only way to get a reliable error indicator (beyond what you get
from the write system call) would be fsync.  And I doubt you want to
call that, purely for performance reasons.

This means that for Linux at least, close_stdout should just call
fflush, not fclose.

Thanks,
Florian

Re: Why does close_stdout close stdout and stderr?

[Florian Weimer]

* Eric Blake:

> On 4/29/19 2:45 PM, Florian Weimer wrote:
>> I get that error checking is important.  But why not just use ferror and
>> fflush?  Closing the streams is excessive and tends to introduce
>> use-after-free issues, as evidenced by the sanitizer workarounds.
>
> If I recall the explanation, at least some versions of NFS do not
> actually flush on fflush(), but wait until close(). If you want to avoid
> data loss and ensure that things written made it to the remote storage
> while detecting every possible indication when an error may have
> prevented that from working, then you have to go all the way through
> close().

Any file system on Linux does this to a varying degree (unlike Solaris
and Windows, I think).  If you want to catch low-level I/O errors, you
need to call fsync after fflush.  And I doubt this is something we want
to do because it will result in bad-looking performance.

But the NFS aspect is somewhat plausible at least.

I can try to figure out if NFS makes a difference for Linux here,
i.e. if there are cases where a write will succeed, but only an
immediately following close will report an error condition that is
known, in principle, even at the time of the write.  A difference
between hard and soft NFS mounts could matter in this context.

Thanks,
Florian

Why does close_stdout close stdout and stderr?

[Florian Weimer]

I get that error checking is important.  But why not just use ferror and
fflush?  Closing the streams is excessive and tends to introduce
use-after-free issues, as evidenced by the sanitizer workarounds.

Thanks,
Florian

Re: new module suggestion: fprintftime-check

[Florian Weimer]

* Bruno Haible:

> Florian Weimer wrote:
>> The standards do not provide a way to report errors for malformed format
>> strings.  I think the current behavior is acceptable, all things
>> considered.
>
> OK, then I'm fine with Assaf's approach to create a new, separate function
> that does only the syntax checking.

How do you plan to keep this aligned with the glibc implementation?

Thanks,
Florian

Re: [RFC] Adding a real HashTable implementation to gnulib

[Florian Weimer]

* Tim Rühsen:

> But I still wonder why
>
>  - nested functions need an executable stack (why does the code have to
> be run on the stack ?)

The static chain pointer needs to be passed to the nest function, but
most ABIs only have a code pointer.  So a trampoline is written to the
stack which sets up the static chain (in a hidden function argument) and
calls the implementation.  The address of the trampoline is used as the
code pointer for the nested function.

>  - there are no efforts to standardize either nested functions or blocks
> (clang has 'blocks' instead of nested functions) or both. (There is a
> C22 panel, but I couldn't find anything useful on their task list.)

You can simply use C++.  C++11 and later have std::function and lambdas.

The standardization of lambdas for C has been abandoned, as far as I
know.

Thanks,
Florian

Re: closed file descriptors on HP-UX

[Florian Weimer]

* Bruno Haible:

> Oh well. Then it's even POSIX compliant. But HP-UX (and possibly
> Windows) is the only platform where things are really like this -
> otherwise we would have seen more platforms where the diffutils
> 'new-file' test fails.

glibc does this as well if the process underwent an AT_SECURE
transition as part of execve; see __libc_check_standard_fds.

Thanks,
Florian

Re: new module suggestion: fprintftime-check

[Florian Weimer]

* Bruno Haible:

> [CCing Florian Weimer.
> Florian, the thread started at
> https://lists.gnu.org/archive/html/bug-gnulib/2018-12/msg00149.html ]
>
> Assaf Gordon wrote:
>> The comment even says:
>>/* Unknown format; output the format, including the '%',
>>   since this is most likely the right thing to do if a
>>   multibyte string has been misparsed.  */
>> 
>> This has been the case since 1996 when strftime.c was imported from libc
>> (gnulib commit afabd949).
>> 
>> I suspect that changing this behavior would be a disruptive
>> backwards-incompatible change (but other opinions are welcomed).
>
> The "security" and "robustness" aspects of software have gained importance
> over the last 22 years, also in domain of glibc.
>
> Florian, Assaf discovered that glibc processing of time format strings
> (strftime) operates according to the garbage-in - garbage-out principle,
> that is, an invalid format string does not get reported to the caller
> but instead produces output that is "most likely the right thing".

Historically, some Lua scripts have relied on strftime not crashing, but
I think this awas fixed on the Lua side a couple of years ago.

The standards do not provide a way to report errors for malformed format
strings.  I think the current behavior is acceptable, all things
considered.

Thanks,
Florian

Re: [PATCH] Add renameat2 function [BZ #17662]

[Florian Weimer]

On 07/04/2018 10:13 PM, Carlos O'Donell wrote:

This is a good suggestion, and I think Florian should work on something
going into the manual to document the behaviour.


We do not have any documentation for the *at functions at present.  I 
find it difficult to document renameat2 without reference to openat and 
a generic description of the AT_* flags.  I feel this is something we 
should tackle after the release.


Once the patch is in, I will propose something for the existing manual 
page, documenting the EINVAL behavior of the glibc wrapper and the 
existence of the gnulib implementation.



You position Gnulib's implementation as having no drawbacks, but this
is not true. The API has a race, and it is something which along with
other similar racy APIs has caused difficult to solve problems a the
distribution level.


And as Joseph pointed out, there is a different emulation strategy with 
a different failure mode (use link and potentially leave behind a 
hard-linked file under both names).


Thanks,
Florian

Re: [PROPOSED] renameatu: rename from renameat2

[Florian Weimer]

On 07/04/2018 04:45 AM, Paul Eggert wrote:

It's looking like Glibc will add a renameat2 function
that is incompatible with Gnulib renameat2; see:
https://sourceware.org/ml/libc-alpha/2018-07/msg00064.html
To help avoid future confusion, rename renameat2 to something else.
Use the name 'renameatu', as the Gnulib function is close to the
Glibc function.  Perhaps someday there will also be a renameat2
Gnulib module, which mimicks the future glibc renameat2, but that
can wait as nobody seems to need such a module now.
* lib/renameatu.c: Rename from lib/renameat2.c.
* lib/renameatu.h: Rename from lib/renameat2.h.
* modules/renameat2: Rename from modules/renameatu.
* modules/renameat2-tests: Rename from modules/renameat2-tests.
All uses of "renameat2" in identifiers or file name
changed to "renameatu", except for two instances in
lib/renameatu.c that deal with the Linux kernel's
renameat2 syscall.


Thank you for being flexible in resolving this.  It's an unfortunate 
situation.


Florian

Re: [PATCH] Add renameat2 function [BZ #17662]

[Florian Weimer]

On 07/04/2018 11:04 AM, Andreas Schwab wrote:

On Jul 03 2018, Paul Eggert  wrote:


Florian Weimer wrote:

Surely that's a gnulib bug because the main reason for the
RENAME_NOREPLACE variant renameat2 was to avoid exactly that race (or
the other race where the file exists under both the old and new path).


No, it's intended behavior, not a bug. GNU mv uses renameat2 with
RENAME_NOREPLACE. mv wants the noreplace semantics on platforms that
support it (currently only recent Linux and macOS kernels); otherwise it
wants exactly that race because that's the best that can be done on other
platforms. If Gnulib renameat2 simply failed with EINVAL because
RENAME_NOREPLACE was not supported, GNU mv would simply use the same racy
fallback that Gnulib renameat2 already uses.

Other GNU applications are similar to GNU mv in this respect.


IMHO we should not repeat the pselect error.  Glibc should provide the
race-free guarantee that RENAME_NOREPLACE gives, so that programs that
need it can use it without fear.


What do you think about the rest of the patch?  Do you think it can be 
committed?


Thanks,
Florian

Re: [PATCH] Add renameat2 function [BZ #17662]

[Florian Weimer]

On 07/02/2018 07:38 PM, Paul Eggert wrote:

Florian Weimer wrote:

Without kernel support, a non-zero argument returns EINVAL, not ENOSYS.
This mirrors what the kernel does for invalid renameat2 flags.


The Gnulib renameat2 function 
<https://www.gnu.org/software/gnulib/MODULES.html#module=renameat2> has 
different semantics with non-zero flags. On GNU/Linux if 
flags==RENAME_NOREPLACE and the Linux syscall fails due to 
EINVAL/ENOSYS/ENOTSUP, Gnulib renameat2 falls back on fstatatting the 
destination, failing if fstatat succeeds, and using ordinary renameat 
otherwise. Of course this implementation has a race condition, but 
Gnulib-using applications like GNU 'mv' prefer this implementation since 
if the kernel doesn't support RENAME_NOREPLACE they'd just fall back on 
fstatat themselves anyway, if renameat2 didn't do that for them.


Surely that's a gnulib bug because the main reason for the 
RENAME_NOREPLACE variant renameat2 was to avoid exactly that race (or 
the other race where the file exists under both the old and new path).


The gnulib function should simply be called something else, not 
renameat2.  The present situation is unfortunate, but I don't think it 
would be an improvement if glibc copies the buggy gnulib behavior.


Thanks,
Florian

Re: malloca, freea are not thread-safe

[Florian Weimer]

On 02/02/2018 11:10 PM, Paul Eggert wrote:
This can cause problems when -fcheck-pointer-bounds is in effect, since 
converting a pointer to uintptr_t and back means that GCC won't connect 
the resulting pointer to the original and this messes up bounds checking 
on the result.


-fcheck-pointer-bounds in GCC doesn't really work.  The existing 
implementation is barely a research prototype (for example, most string 
functions are not protected by it), and I don't think anyone knows how 
to make it thread-safe.  Its existence shouldn't be used as a guidance 
for anything, really.


Thanks,
Florian

Re: malloca, freea are not thread-safe

[Florian Weimer]

On 01/11/2018 05:26 AM, Bruno Haible wrote:

Another idea is to add some header (like the current implementation does),
but instead of storing a marker only in the malloc case, store a different
marker also in the alloca case. This should be done through a GCC statement
expression.
=> Should work with __builtin_alloca.


Could we please fix this issue along those lines?  Thanks.

Florian

malloca, freea are not thread-safe

[Florian Weimer]

The mmalloca function used to implement malloca accesses a static global 
array without synchronization:


#define HASH_TABLE_SIZE 257
static void * mmalloca_results[HASH_TABLE_SIZE];
…
mmalloca (size_t n)
{
…
/* Enter p into the hash table.  */
  slot = (uintptr_t) p % HASH_TABLE_SIZE;
  h->next = mmalloca_results[slot];
  mmalloca_results[slot] = p;

freea also causes valgrind warnings because it contains an out-of-bounds 
access.  This is very undesirable because it will cause programmers to 
miss real bugs.


This code has been copied into libunistring and results in a thread 
safety hazard there.


Thanks,
Florian

Re: [PATCH 2/3] scratch_buffer: new module

[Florian Weimer]

On 09/02/2017 01:33 AM, Paul Eggert wrote:

> +AC_CHECK_FUNCS_ONCE([__libc_scratch_buffer_grow])

This configure test is invalid because __libc_scratch_buffer_grow is a
GLIBC_PRIVATE symbol not part of the ABI.

You really need to rename those __libc_* functions because they are in
the internal glibc namespace.

Furthermore, struct scratch_buffer depends on a GNU C extension:
assignment to an object changes its dynamic type.  It is not standard C.
 Standard C only allows one way to allocate untyped memory, and that is
malloc.

Thanks,
Florian

Re: __typeof__ does not work as expected with XLC compiler on AIX 5.2

[Florian Weimer]

On 10/28/2016 05:13 PM, TestRealTime . wrote:

Hello,

I would like to report about two (probably, self-connected) bugs in the
__typeof__ C function.
These bugs are reproduced when I am trying to compile a simple code with
XLC compiler on AIX 5.2. Please, look at the code snippets below.

1. typeof_example_1.c
long Func(int x) { return x; }
int main()
{
int y, z = 1;
y = (__typeof__(Func(z)))0;
return 0;
}

cc_r typeof_example_1.c
"typeof_example_1.c", line 5.26: 1506-045 (S) Undeclared identifier z.

The 'z' variable is obviously defined, so "Undeclared identifier" message
is wrong.


Do you have any evidence that the compiler supports __typeof__ at all? 
Maybe the compiler treats __typeof__ as an implicitly declared function.



What is interesting, we use cc_r, but not xlc binary in our build system
(it is probably due to historical reasons and persons who can answer me why
exactly cc_r have gone many years ago). But with xlc both code snippets
also fail, however with other error messages (no matter, 32 or 64bit).
xlc typeof_example_1.c
"typeof_example_1.c", line 5.30: 1506-275 (S) Unexpected text integer
constant encountered.
xlc typeof_example_2.c
"typeof_example_2.c", line 5.41: 1506-275 (S) Unexpected text z encountered.
"typeof_example_2.c", line 5.45: 1506-275 (S) Unexpected text integer
constant encountered.


Those syntax errors are consistent with not supporting __typeof__ at 
all.  The expression parts which contain __typeof__ are not recognized 
as types, and so the expression is not recognized as a cast.


Florian