Mark H Weaver <m...@netris.org> skribis: > l...@gnu.org (Ludovic Courtès) writes:
[...] >> We could use a self symlink, or we could use something like >> <http://git.savannah.gnu.org/cgit/guix.git/commit/?id=9d50da70608de32d9db0c29859caec6f2cddb95f>. >> >> Mark, WDYT? >> >> What remains to be seen is how many packages are affected by this issue, >> and whether a generic solution needs to be found. > > Unfortunately, it is too widespread. As I just pointed out in > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24712#13 > > Among the many packages that include these obfuscated store references, > one is 'glibc-final'. My attempt to graft 'bash' in 'master' to fix > CVE-2016-0634 and CVE-2016-7543 has resulted in a system where I cannot > build *any* derivation, because 'guile-final' crashes during boot while > its 'glibc-final' tries to find its 'gconv' modules in the ungrafted > 'glibc-final', which is not available in the build environment. Out of curiosity, Guile crashes while loading iconv modules, right? This is surprising because the guile-for-build is always the ungrafted ‘guile-final’ (see ‘gnu-build’ in (guix build-system gnu)). > So, if our approach is to use -fno-builtin-strcpy, then we will have to > apply it system-wide, and rebuild all of 'core-updates' from scratch. Another approach would be to patch GCC, specifically ‘expand_movstr’ in gcc/builtins.c, which is the part responsible for this optimization, along these lines (untested):
--- gcc-5.3.0/gcc/builtins.c.orig 2016-10-18 10:45:35.042826368 +0200 +++ gcc-5.3.0/gcc/builtins.c 2016-10-18 10:50:46.080616285 +0200 @@ -3470,6 +3470,19 @@ expand_builtin_mempcpy_args (tree dest, # define CODE_FOR_movstr CODE_FOR_nothing #endif +/* Return true if STR is a string denoting a "/gnu/store" file name. */ + +static bool +store_reference_p (tree str) +{ + const char *store; + + store = getenv ("NIX_STORE") ?: "/gnu/store"; + + return (TREE_STRING_LENGTH (str) > strlen (store) + && strncmp (TREE_STRING_POINTER (str), store, strlen (store))); +} + /* Expand into a movstr instruction, if one is available. Return NULL_RTX if we failed, the caller should emit a normal call, otherwise try to get the result in TARGET, if convenient. If ENDP is 0 return the @@ -3484,7 +3497,9 @@ expand_movstr (tree dest, tree src, rtx rtx dest_mem; rtx src_mem; - if (!HAVE_movstr) + /* When SRC denotes a store item, do not perform any optimization. See + <http://bugs.gnu.org/24703> for background. */ + if (!HAVE_movstr || store_reference_p (src)) return NULL_RTX; dest_mem = get_memory_rtx (dest, NULL);
WDYT? In the meantime, we need a workaround. The only option I can think of is to retain a reference to the ungrafted item by adding a symlink to it, like:
diff --git a/guix/grafts.scm b/guix/grafts.scm index 80ae27e..4de19ae 100644 --- a/guix/grafts.scm +++ b/guix/grafts.scm @@ -127,7 +127,14 @@ recursively applied to dependencies of DRV." files)) (match %outputs (((names . files) ...) - files)))))) + files))) + + (for-each (match-lambda + ((name . file) + (symlink file + (string-append (assoc-ref %outputs name) + "/ungrafted")))) + old-outputs)))) (define add-label (cut cons "x" <>))
> I've been investigating another approach: to enhance our scanner and > grafter to handle these 8-byte-chunked references. I believe it is > feasible, but only if we abandon the ability to change the file names of > grafts outside of the hash. The reason is that the hash portion of > store references are surrounded by enough other known characters on both > sides that the hash portion is almost always contained entirely within > 8-byte chunks. I think that would add complexity, would make grafting slower, and abandoning the ability to change file names would be a regression. So I’m more in favor of a GCC patch like above, or another compilation tweak. WDYT? Thanks, Ludo’.