Re: 01/01: gnu: glibc/linux: Fix runtime crashes on i686 systems.

[Mark H Weaver]

Hi Ricardo,

address@hidden (Ricardo Wurmus) writes:

> rekado pushed a commit to branch master
> in repository guix.
>
> commit b2fd8f63679aa4f244c36fdca62f23c00b8eded9
> Author: Ricardo Wurmus <address@hidden>
> Date:   Wed Apr 26 13:03:48 2017 +0200
>
>     gnu: glibc/linux: Fix runtime crashes on i686 systems.
>     
>     * gnu/packages/patches/glibc-memchr-overflow-i686.patch: New file.
>     * gnu/local.mk (dist_patch_DATA): Add it.
>     * gnu/packages/commencement.scm 
> (glibc-final-with-bootstrap-bash)[native-inputs]:
>     Add the patch conditionally for i686 systems.
>     * gnu/packages/base.scm (glibc/linux)[native-inputs]: Add the patch
>     conditionally for i686 systems.
>     [arguments]: Apply the patch conditionally on i686 systems.

Thanks very much for this.  I noticed a minor issue with this patch:

> diff --git a/gnu/packages/base.scm b/gnu/packages/base.scm
> index 9fcca45..6dc9e97 100644
> --- a/gnu/packages/base.scm
> +++ b/gnu/packages/base.scm
> @@ -666,6 +666,16 @@ store.")
>                          ;; 4.7.1.
>                          ((" -lgcc_s") ""))
>  
> +                      ;; Apply patch only on i686.
> +                      ;; TODO: Move the patch to 'patches' in the next 
> update cycle.
> +                      ,@(if (string-prefix? "i686" (or 
> (%current-target-system)
> +                                                       (%current-system)))
> +                            `(zero? (system* "patch" "-p1" "--force"
> +                                             "--input"
> +                                             (assoc-ref native-inputs
> +                                                        
> "glibc-memchr-overflow-i686.patch")))

The `(zero? ...) subexpression is incorrect for two reasons:

The first issue is that you need another layer of parentheses,
i.e. `((zero? ...)), to generate the code as you presumably intended.
As it is, the generated code looks like this:

--8<---------------cut here---------------start------------->8---
  (substitute* "Makeconfig"
    ;; According to
    ;; <http://www.linuxfromscratch.org/lfs/view/stable/chapter05/glibc.html>,
    ;; linking against libgcc_s is not needed with GCC
    ;; 4.7.1.
    ((" -lgcc_s") ""))

  zero?
  (system* "patch" "-p1" "--force"
           "--input"
           (assoc-ref native-inputs
                      "glibc-memchr-overflow-i686.patch"))
--8<---------------cut here---------------end--------------->8---

Since the isolated variable reference 'zero?' has no side effects and
the result is discarded, it has no effect and is presumably optimized
out.  'system*' returns an integer.  All integers are interpreted as
"true" in Scheme, including zero.

The other problem is that (zero? (system* ...)) does not have the
desired effect, because it is not the final expression in the outer
body, and therefore its result is discarded and errors are not flagged.

What's needed here is to explicitly raise an exception if 'system*'
returns a non-zero result, e.g. something like this (untested):

> +                      ;; Apply patch only on i686.
> +                      ;; TODO: Move the patch to 'patches' in the next 
> update cycle.
> +                      ,@(if (string-prefix? "i686" (or 
> (%current-target-system)
> +                                                       (%current-system)))
> +                            `((unless (zero? (system* "patch" "-p1" "--force"
> +                                                      "--input"
> +                                                      (assoc-ref 
> native-inputs
> +                                                                 
> "glibc-memchr-overflow-i686.patch")))
> +                                (error "patch failed for 
> glibc-memchr-overflow-i686.patch")))

This reminds me of something I've felt for a while: that we should stop
using 'system*' in most places, and instead use a variant of 'system*'
that automatically raises an exception in case of a non-zero result
code, but that's a subject for another thread.

Thoughts?

     Mark