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 <meyering@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 739752@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: