On Thu, Mar 13, 2014 at 7:22 PM, Pádraig Brady <[email protected]> wrote:
> On 03/14/2014 01:42 AM, Jim Meyering wrote:
>> From a6d2db8b6dfe15344aba4aefe9545eb3a4876d45 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <[email protected]>
>> 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 [email protected].
>
> 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 <[email protected]>
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 [email protected].
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 [email protected]
Eric Pemente [email protected]
Eric S. Raymond [email protected]
Erik Bennett [email protected]
+Erik Bernstein [email protected]
Erik Corry [email protected]
Felix Lee [email protected]
Felix Rauch Valenti [email protected]
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
