On Thu, Mar 13, 2014 at 7:22 PM, Pádraig Brady <p...@draigbrady.com> wrote: > On 03/14/2014 01:42 AM, Jim Meyering wrote: >> From a6d2db8b6dfe15344aba4aefe9545eb3a4876d45 Mon Sep 17 00:00:00 2001 >> From: Jim Meyering <meyer...@fb.com> >> Date: Thu, 13 Mar 2014 17:05:04 -0700 >> Subject: [PATCH] ln: with -sr, don't segfault for a TARGET of '' >> >> Prior to this change, "ln -sr '' F" would segfault, attempting >> to read path2[1] in relpath.c's path_common_prefix function. >> This problem arises whenever canonicalize_filename_mode returns >> NULL. >> * src/ln.c (convert_abs_rel): Call relpath only when >> both canonicalize_filename_mode calls return non-NULL. >> * tests/ln/relative.sh: Add a test to trigger this failure. >> * THANKS.in: List reporter's name/address. >> * NEWS (Bug fixes): Mention it. >> Reported by Erik Bernstein in 739...@bugs.debian.org. > > We can amend with the now allocated: > > Fixes http://bugs.gnu.org/17010
Done. >> diff --git a/NEWS b/NEWS ... >> + ln -sr '' F no longer segfaults: now it fails with the expected diagnostic > > Probably should add: > > [bug introduced with the --relative feature in coreutils-8.16] Definitely. Thanks. >> diff --git a/src/ln.c b/src/ln.c >> index aab9cf2..6726699 100644 >> --- a/src/ln.c >> +++ b/src/ln.c >> @@ -149,13 +149,17 @@ convert_abs_rel (const char *from, const char *target) >> char *realdest = canonicalize_filename_mode (targetdir, CAN_MISSING); >> char *realfrom = canonicalize_filename_mode (from, CAN_MISSING); > > Interesting. So canonicalize_filename_mode() can fail in this case, > even with CAN_MISSING. It's unexpected that c_f_m() sets errno=ENOENT > when CAN_MISSING is set. I wonder should we change that instead > in gnulib? With CAN_MISSING I would expect this function to work > on arbitrary strings, including the empty string. What would you have c_f_m("", CAN_MISSING) return? I know of no absolute name corresponding to the dot-relative empty string. >> diff --git a/tests/ln/relative.sh b/tests/ln/relative.sh >> +# Expect this to fail with exit status 1. >> +# Prior to coreutils-8.23, it would segfault. >> +ln -sr '' F >> +test $? = 1 || fail=1 > > Won't the ln succeed on FreeBSD as per: > http://bugs.gnu.org/13447 You're right. Good catch. Adjusted as well. Thanks for the speedy and thorough review. Here's a revised patch:
From 0093ac8d57a0f1a16fd09d98f6a524dddb6053e7 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@fb.com> Date: Thu, 13 Mar 2014 17:05:04 -0700 Subject: [PATCH] ln: with -sr, don't segfault for a TARGET of '' Prior to this change, "ln -sr '' F" would segfault, attempting to read path2[1] in relpath.c's path_common_prefix function. This problem arises whenever canonicalize_filename_mode returns NULL. * src/ln.c (convert_abs_rel): Call relpath only when both canonicalize_filename_mode calls return non-NULL. * tests/ln/relative.sh: Add a test to trigger this failure. * THANKS.in: List reporter's name/address. * NEWS (Bug fixes): Mention it. Reported by Erik Bernstein in 739...@bugs.debian.org. Fixes http://bugs.gnu.org/17010. --- NEWS | 3 +++ THANKS.in | 1 + src/ln.c | 16 ++++++++++------ tests/ln/relative.sh | 5 +++++ 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/NEWS b/NEWS index 62966b2..35d48e5 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,9 @@ GNU coreutils NEWS -*- outline -*- it would display an error, requiring --no-dereference to avoid the issue. [bug introduced in coreutils-5.3.0] + ln -sr '' F no longer segfaults. Now works as expected. + [bug introduced with the --relative feature in coreutils-8.16] + shuf --repeat no longer dumps core if the input is empty. [bug introduced with the --repeat feature in coreutils-8.22] diff --git a/THANKS.in b/THANKS.in index 561d18c..2670265 100644 --- a/THANKS.in +++ b/THANKS.in @@ -193,6 +193,7 @@ Eric G. Miller e...@jps.net Eric Pemente peme...@northpark.edu Eric S. Raymond e...@snark.thyrsus.com Erik Bennett benn...@cvo.oneworld.com +Erik Bernstein e...@fscking.org Erik Corry e...@kroete2.freinet.de Felix Lee f...@teleport.com Felix Rauch Valenti fra...@cse.unsw.edu.au diff --git a/src/ln.c b/src/ln.c index aab9cf2..6726699 100644 --- a/src/ln.c +++ b/src/ln.c @@ -149,13 +149,17 @@ convert_abs_rel (const char *from, const char *target) char *realdest = canonicalize_filename_mode (targetdir, CAN_MISSING); char *realfrom = canonicalize_filename_mode (from, CAN_MISSING); - /* Write to a PATH_MAX buffer. */ - char *relative_from = xmalloc (PATH_MAX); - - if (!relpath (realfrom, realdest, relative_from, PATH_MAX)) + char *relative_from = NULL; + if (realdest && realfrom) { - free (relative_from); - relative_from = NULL; + /* Write to a PATH_MAX buffer. */ + relative_from = xmalloc (PATH_MAX); + + if (!relpath (realfrom, realdest, relative_from, PATH_MAX)) + { + free (relative_from); + relative_from = NULL; + } } free (targetdir); diff --git a/tests/ln/relative.sh b/tests/ln/relative.sh index 7636695..5cf280a 100755 --- a/tests/ln/relative.sh +++ b/tests/ln/relative.sh @@ -45,4 +45,9 @@ mkdir web ln -sr latest web/latest test $(readlink web/latest) = '../release2' || fail=1 +# Expect this to fail with exit status 1, or to succeed quietly (freebsd). +# Prior to coreutils-8.23, it would segfault. +ln -sr '' F +case $? in [01]) ;; *) fail=1;; esac + Exit $fail -- 1.9.0.167.g384364b