Re: dl-sysdep Problem

[John Reiser <jreiser at BitWagon dot com>]

Andreas Jaeger <aj@suse.de> wrote:
> the brk (0) looks quite strange to me.

There is a global variable __curbrk (with weak alias ___brk_addr)
which gets set to its first value by calling __brk(0).
See sysdeps/unix/sysv/linux/i386/brk.c line 46.

However, in __sbrk()  sysdeps/generic/sbrk.c line 41:
  if (__curbrk == NULL || __libc_multiple_libcs)
    if (__brk (0) < 0)          /* Initialize the break.  */
      return (void *) -1;

So the __brk(0) in sysdeps/unix/sysv/linux/dl-sysdep.c line 32
also appears to be redundant: the test for __curbrk==NULL will
be true during the first call to __sbrk(), thus causing __brk(0)
to be called then.  frob_brk() could become empty.
Tracking down whether this is true in all architectures
would take some effort.



Andreas Jaeger wrote:
> 
> John writes in PR libc/2312:
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> There are two problems with this code from
> ----- glibc-2.2.3/sysdeps/unix/sysv/linux/dl-sysdep.c:
> if (__sbrk (0) == &_end)
> /* The dynamic linker was run as a program, and so the initial break
> starts just after our bss, at &_end. The malloc in dl-minimal.c
> will consume the rest of this page, so tell the kernel to move the
> break up that far. When the user program examines its break, it
> will see this new value and not clobber our data. */
> __sbrk (_dl_pagesize - ((&_end - (void *) 0) & _dl_pagesize));
> -----
> The code does not do what the comment says; there is a "- 1" missing
> from the usual formulation. In detail: Note that the form of the
> argument is A - (B & A) , which is equivalent to A &~ B .
> In this case, A is _dl_pagesize , which is a power of 2.
> Thus the value of the argument to __sbrk is either 0 or _dl_pagesize ,
> (either the &~ clears the '1' bit, or the &~ leaves it untouched),
> and in fact is 0 unless &_end designates an even page.
> So after the return from __sbrk, the brk will be somewhere on
> an odd-numbered page, and will not be on a page boundary unless
> &_end is already on a page boundary.
> 
> However, the action is not needed anyway, because [if it is removed, then]
> it is subsumed by correct code which executes later:
> ----- sysdeps/generic/dl-sysdep.c:199
> if (__sbrk (0) == &_end)
> /* The dynamic linker was run as a program, and so the initial break
> starts just after our bss, at &_end. The malloc in dl-minimal.c
> will consume the rest of this page, so tell the kernel to move the
> break up that far. When the user program examines its break, it
> will see this new value and not clobber our data. */
> __sbrk (_dl_pagesize - ((&_end - (void *) 0) & (_dl_pagesize - 1)));
> -----
> Note the " - 1" near the righthand end of the line, which differs
> from sysdeps/unix/sysv/linux/dl-sysdep.c .
> The incorrect code in linux/dl-sysdep.c is invoked from
> ----- sysdeps/generic/dl-sysdep.c:181
> #ifdef DL_SYSDEP_INIT
> DL_SYSDEP_INIT;
> #endif
> 
> #ifdef DL_PLATFORM_INIT
> DL_PLATFORM_INIT;
> #endif
> -----
> which is just a few lines earlier than the correct code which recovers
> from the mistake.
> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> I therefore suggest the appended patch but ask to double check this -
> the brk (0) looks quite strange to me.
> 
> Ok to commit?
> 
> Andreas
> 
> 2001-07-01  Andreas Jaeger  <aj@suse.de>
> 
>         * sysdeps/unix/sysv/linux/dl-sysdep.c (frob_brk): Remove duplicate
>         sbrk.
>         Closes PR libc/2312, suggested by John Reiser <jreiser@BitWagon.com>.
> 
> ============================================================
> Index: sysdeps/unix/sysv/linux/dl-sysdep.c
> --- sysdeps/unix/sysv/linux/dl-sysdep.c 1997/07/26 02:33:02     1.3
> +++ sysdeps/unix/sysv/linux/dl-sysdep.c 2001/07/01 12:24:34
> @@ -1,5 +1,5 @@
>  /* Dynamic linker system dependencies for Linux.
> -   Copyright (C) 1995, 1997 Free Software Foundation, Inc.
> +   Copyright (C) 1995, 1997, 2001 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
> 
>     The GNU C Library is free software; you can redistribute it and/or
> @@ -27,16 +27,7 @@
>  static inline void
>  frob_brk (void)
>  {
> -  extern size_t _dl_pagesize;
> -  extern void _end;
>    __brk (0);                   /* Initialize the break.  */
> -  if (__sbrk (0) == &_end)
> -    /* The dynamic linker was run as a program, and so the initial break
> -       starts just after our bss, at &_end.  The malloc in dl-minimal.c
> -       will consume the rest of this page, so tell the kernel to move the
> -       break up that far.  When the user program examines its break, it
> -       will see this new value and not clobber our data.  */
> -    __sbrk (_dl_pagesize - ((&_end - (void *) 0) & _dl_pagesize));
>  }
> 
>  #include <sysdeps/generic/dl-sysdep.c>
> 
> --
>  Andreas Jaeger
>   SuSE Labs aj@suse.de
>    private aj@arthur.inka.de
>     http://www.suse.de/~aj

-- 
John Reiser, jreiser@BitWagon.com