bug#17010: Bug#739752: coreutils: ln segfaults when run with --relative and an empty target
On Fri, Mar 14, 2014 at 9:27 AM, Jim Meyering wrote:
> On Fri, Mar 14, 2014 at 4:49 AM, Pádraig Brady wrote:
> ...
>> Hence since c_f_m() can validly fail even with CAN_MISSING,
>> I agree your patch is correct.
>>
>> Please push.
>
> Done.
Hmm... For the record, I also pushed (accidentally) two additional changes that
had been languishing in the affected repository. Here they are:
From 42d2377b813507d50f5a67076bb20134ceb2fc10 Mon Sep 17 00:00:00 2001
From: Jim Meyering
Date: Mon, 5 Aug 2013 07:28:34 -0700
Subject: [PATCH 1/2] scripts: autotools-install: update
* scripts/autotools-install: Update version numbers of latest
automake and gettext packages.
---
scripts/autotools-install | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/autotools-install b/scripts/autotools-install
index 403644f..c6a8e36 100755
--- a/scripts/autotools-install
+++ b/scripts/autotools-install
@@ -24,9 +24,9 @@ tarballs='
http://pkgconfig.freedesktop.org/releases/pkg-config-0.28.tar.gz
http://ftp.gnu.org/gnu/m4/m4-1.4.16.tar.gz
http://ftp.gnu.org/gnu/autoconf/autoconf-2.69.tar.gz
- http://ftp.gnu.org/gnu/automake/automake-1.12.6.tar.gz
+ http://ftp.gnu.org/gnu/automake/automake-1.14.tar.gz
http://ftp.gnu.org/gnu/libtool/libtool-2.4.2.tar.gz
- http://ftp.gnu.org/gnu/gettext/gettext-0.18.2.tar.gz
+ http://ftp.gnu.org/gnu/gettext/gettext-0.18.3.tar.gz
'
usage() {
--
1.9.0.167.g384364b
From 4f2118221777dc915f2df4e2ed3b65c68301 Mon Sep 17 00:00:00 2001
From: Jim Meyering
Date: Mon, 18 Nov 2013 10:13:06 -0800
Subject: [PATCH 2/2] maint: avoid "attribute-const"-suggesting warning from
gcc
* gl/lib/fadvise.c: Use a pragma to turn off this warning option:
-Wsuggest-attribute=const. Without this change, building with
--enable-gcc-warnings would evoke this error:
lib/fadvise.c:25:1: error: function might be candidate for\
attribute 'const' [-Werror=suggest-attribute=const]
---
gl/lib/fadvise.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/gl/lib/fadvise.c b/gl/lib/fadvise.c
index 3456ce1..562f1eb 100644
--- a/gl/lib/fadvise.c
+++ b/gl/lib/fadvise.c
@@ -14,6 +14,12 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see
bug#17010: Bug#739752: coreutils: ln segfaults when run with --relative and an empty target
On Fri, Mar 14, 2014 at 4:49 AM, Pádraig Brady wrote: ... > Hence since c_f_m() can validly fail even with CAN_MISSING, > I agree your patch is correct. > > Please push. Done.
bug#17010: Bug#739752: coreutils: ln segfaults when run with --relative and an empty target
On 03/14/2014 03:44 AM, Jim Meyering wrote:
> On Thu, Mar 13, 2014 at 7:22 PM, Pádraig Brady wrote:
>> 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.
Since with CAN_MISSING we should be just degenerating to string processing,
I would think it slightly better to return xstrdup(""),
to avoid special casing that in each caller.
I also notice that c_f_m() can return ENOMEM (from areadlink_with_size).
It's arguable that we should xalloc_die() within c_f_m()
for consistency in that case.
However I also notice that c_f_m("", CAN_MISSING) can return ENOENT
if the current working dir is unlinked. I'm not sure
there is anything else we could do within c_f_m() to handle that.
Hence since c_f_m() can validly fail even with CAN_MISSING,
I agree your patch is correct.
Please push.
thanks!
Pádraig.
bug#17010: Bug#739752: coreutils: ln segfaults when run with --relative and an empty target
On Thu, Mar 13, 2014 at 7:22 PM, Pádraig Brady wrote:
> On 03/14/2014 01:42 AM, Jim Meyering wrote:
>> From a6d2db8b6dfe15344aba4aefe9545eb3a4876d45 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering
>> 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
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 Pementepeme...@northpark.edu
Eric S. Raymond e...@snark.thyrsus.com
Erik Bennettbenn...@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 =
bug#17010: Bug#739752: coreutils: ln segfaults when run with --relative and an empty target
On 03/14/2014 01:42 AM, Jim Meyering wrote:
> From a6d2db8b6dfe15344aba4aefe9545eb3a4876d45 Mon Sep 17 00:00:00 2001
> From: Jim Meyering
> 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
> diff --git a/NEWS b/NEWS
> index 62966b2..b3ad65c 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -25,6 +25,8 @@ 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 it fails with the expected diagnostic
Probably should add:
[bug introduced with the --relative feature in coreutils-8.16]
> 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.
>
> - /* 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
> +# 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
thanks,
Pádraig.
bug#17010: Bug#739752: coreutils: ln segfaults when run with --relative and an empty target
On Sat, Feb 22, 2014 at 1:57 AM, Erik Bernstein wrote:
> Package: coreutils
> Version: 8.21-1
> Severity: normal
>
> Hi,
>
> when ln is run with --relative --symbolic and and empty string as the
> target, it ungracefully dies with a segmentation fault. The memory
> violation appears to happen in src/relpath.c:38 when the two input paths
> are checked for leading slashes:
>
> if ((path1[1] == '/') != (path2[1] == '/'))
>
> How to reproduce:
> [1] Open a terminal
> [2] run: ln -sr '' foobar
>
> Result: segmentation fault ln -sr '' foobar
> Expected result: Some kind of error message
...
Thank you for the bug report!
That also affected the very latest code in git.
Here is a patch:
From a6d2db8b6dfe15344aba4aefe9545eb3a4876d45 Mon Sep 17 00:00:00 2001
From: Jim Meyering
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.
---
NEWS | 2 ++
THANKS.in| 1 +
src/ln.c | 16 ++--
tests/ln/relative.sh | 5 +
4 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/NEWS b/NEWS
index 62966b2..b3ad65c 100644
--- a/NEWS
+++ b/NEWS
@@ -25,6 +25,8 @@ 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 it fails with the expected diagnostic
+
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 Pementepeme...@northpark.edu
Eric S. Raymond e...@snark.thyrsus.com
Erik Bennettbenn...@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..e8c25dc 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.
+# Prior to coreutils-8.23, it would segfault.
+ln -sr '' F
+test $? = 1 || fail=1
+
Exit $fail
--
1.9.0.167.g384364b
