Re: segfault bb_make_directory + dirname with musl

[Tito]

On 11/30/2016 11:52 PM, Denys Vlasenko wrote:

On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  wrote:

The following commands cause busybox to segfault on musl-based systems.

$ install -D a /
$ install -D a /b
$ install -D a /b/

This happens because the code in

https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196

passes the result of dirname() to bb_make_directory() which modifies its
contents. For paths of the above forms, musl's dirname returns a string
literal "/" which shouldn't be modified.

See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c

There are a few other occurrences of the code shown above, but I've not
checked to see if they could be made to segfault.


Does this fix the problem?

/* Bypass leading non-'/'s and then subsequent '/'s */
while (*s) {
if (*s == '/') {
do {
++s;
} while (*s == '/');
c = *s; /* Save the current char */
added line==>   if (c)
*s = '\0'; /* and
replace it with nul */
break;
___

Hi,
don't know if this could be useful, but reading this thread inspired
me to write dirname and basename replacement functions for busybox
that return a malloced string that could be modified by the caller.
The functions seem to work nice as standalone program and both
pass the tests as in the man 3 basename examples and a few more
added by me.

/*  Man 3 BASENAME examples (taken from SUSv2)
path   dirname   basename
   /usr/lib   /usr  lib
   /usr/  / usr
   usr. usr
   /  / /
   .  . .
   .. . ..
 Added examples:
   usr/lib   usr   lib
   usr/lib/  usr   lib
   usr/   .usr
   /usr   .usr
   /a/b/c /a/b  c
   /a/b/c//a/b  c
   a/b/c  a/b   c
   a/b/c/ a/b   c
   // / /
   //// /
      / /
   '/a/b/ '   /a/b ' '
*/

The functions look like this:

char* bb_basename_malloc(const char *name)
{
const char *p1 = NULL;
const char *p2 = NULL;
const char *s = name;
char *last = last_char_is(s, '/');

while (*s) {
if (*s == '/') {
p2 = p1;
p1 = s;
}
s++;
}
if (last) {
if(p2)
name = p2 + 1;
} else if (p1) {
name = p1 + 1;
}
return xstrndup(name, strlen(name) - (last && last != name));
}

char* bb_dirname_malloc(const char *name)
{
int len = 0;
const char *p1 = NULL;
const char *p2 = NULL;
const char *s = name;

while (*s) {
if (*s == '/' && ((s[1] && s[1] != '/') || s == name)) {
p2 = p1;
p1 = s;
}
s++;
}
if (!p2 && !p1)
return xstrdup(".");
if (p1 == name && !p2)
return xstrdup("/");
if (p1)
len  =  strlen(p1);
return xstrndup(name, strlen(name) - len);
}

Attached you will find the standalone version to test the functions.
Hints, critics, improvements are welcome.

Ciao,
Tito
#include 
#include 
#include 

char* xstrndup(const char *s, int n)
{
	int m;
	char *t;

//	if (ENABLE_DEBUG && s == NULL)
//		bb_error_msg_and_die("xstrndup bug");

	/* We can just xmalloc(n+1) and strncpy into it, */
	/* but think about xstrndup("abc", 1) wastage! */
	m = n;
	t = (char*) s;
	while (m) {
		if (!*t) break;
		m--;
		t++;
	}
	n -= m;
	t = malloc(n + 1);
	t[n] = '\0';

	return memcpy(t, s, n);
}

char* xstrdup(const char *s)
{
	char *t;

	if (s == NULL)
		return NULL;

	t = strdup(s);

	if (t == NULL)
		exit(1);

	return t;
}

char* last_char_is(const char *s, int c)
{
	if (s && *s) {
		size_t sz = strlen(s) - 1;
		s += sz;
		if ( (unsigned char)*s == c)
			return (char*)s;
	}
	return NULL;
}

char* bb_basename_malloc(const char *name)
{
	const char *p1 = NULL;
	const char *p2 = NULL;
	const char *s = name; 
	char *last = last_char_is(s, '/');

	while (*s) {
		if (*s == '/') {
			p2 = p1;
			p1 = s;
		}
		s++;
	}
	if (last) {
		if(p2)
			name = p2 + 1;
	} else if (p1) {
		name = p1 + 1;
	}
	return xstrndup(name, strlen(name) - (last && last != name));
}

char* bb_dirname_malloc(const char *name)
{
	int len = 0;
	const char *p1 = NULL;
	const char *p2 = NULL;
	const char *s = name;
	
	while (*s) {
		if (*s == '/' && ((s[1] 

Re: segfault bb_make_directory + dirname with musl

[Daniel Sabogal]

On Sun, Dec 4, 2016 at 4:43 AM, Denys Vlasenko  wrote:
> On Sun, Dec 4, 2016 at 3:45 AM, Daniel Sabogal  wrote:
>> On Thu, Dec 1, 2016 at 3:13 PM, Daniel Sabogal  wrote:
>>> On Wed, Nov 30, 2016 at 5:52 PM, Denys Vlasenko
>>>  wrote:
 On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  
 wrote:
> The following commands cause busybox to segfault on musl-based systems.
>
> $ install -D a /
> $ install -D a /b
> $ install -D a /b/
>
> This happens because the code in
>
> https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196
>
> passes the result of dirname() to bb_make_directory() which modifies its
> contents. For paths of the above forms, musl's dirname returns a string
> literal "/" which shouldn't be modified.
>
> See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c
>
> There are a few other occurrences of the code shown above, but I've not
> checked to see if they could be made to segfault.

 Does this fix the problem?

 /* Bypass leading non-'/'s and then subsequent 
 '/'s */
 while (*s) {
 if (*s == '/') {
 do {
 ++s;
 } while (*s == '/');
 c = *s; /* Save the current char */
 added line==>   if (c)
 *s = '\0'; /* and
 replace it with nul */
 break;
>>>
>>> This does prevent the segfault, but I'm not sure if depending on being able 
>>> to
>>> modify the result of dirname() is reliable.
>>
>> https://git.busybox.net/busybox/commit/?id=cf2600c3661c11491a838ef29733583afb6ad968
>>
>> There are other occurrences of dirname + bb_make_directory that may have
>> this issue.
>>
>> The following also segfaults.
>>
>> $ cp --parents a /
>
> Indeed.
>
> I moved the check into bb_make_directory(), please try now.

I suppose the issue is fixed. The above commands no longer result in a segfault.

Thanks,
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: segfault bb_make_directory + dirname with musl

[Denys Vlasenko]

On Sun, Dec 4, 2016 at 3:45 AM, Daniel Sabogal  wrote:
> On Thu, Dec 1, 2016 at 3:13 PM, Daniel Sabogal  wrote:
>> On Wed, Nov 30, 2016 at 5:52 PM, Denys Vlasenko
>>  wrote:
>>> On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  
>>> wrote:
 The following commands cause busybox to segfault on musl-based systems.

 $ install -D a /
 $ install -D a /b
 $ install -D a /b/

 This happens because the code in

 https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196

 passes the result of dirname() to bb_make_directory() which modifies its
 contents. For paths of the above forms, musl's dirname returns a string
 literal "/" which shouldn't be modified.

 See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c

 There are a few other occurrences of the code shown above, but I've not
 checked to see if they could be made to segfault.
>>>
>>> Does this fix the problem?
>>>
>>> /* Bypass leading non-'/'s and then subsequent '/'s 
>>> */
>>> while (*s) {
>>> if (*s == '/') {
>>> do {
>>> ++s;
>>> } while (*s == '/');
>>> c = *s; /* Save the current char */
>>> added line==>   if (c)
>>> *s = '\0'; /* and
>>> replace it with nul */
>>> break;
>>
>> This does prevent the segfault, but I'm not sure if depending on being able 
>> to
>> modify the result of dirname() is reliable.
>
> https://git.busybox.net/busybox/commit/?id=cf2600c3661c11491a838ef29733583afb6ad968
>
> There are other occurrences of dirname + bb_make_directory that may have
> this issue.
>
> The following also segfaults.
>
> $ cp --parents a /

Indeed.

I moved the check into bb_make_directory(), please try now.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: segfault bb_make_directory + dirname with musl

[Daniel Sabogal]

On Thu, Dec 1, 2016 at 3:13 PM, Daniel Sabogal  wrote:
> On Wed, Nov 30, 2016 at 5:52 PM, Denys Vlasenko
>  wrote:
>> On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  wrote:
>>> The following commands cause busybox to segfault on musl-based systems.
>>>
>>> $ install -D a /
>>> $ install -D a /b
>>> $ install -D a /b/
>>>
>>> This happens because the code in
>>>
>>> https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196
>>>
>>> passes the result of dirname() to bb_make_directory() which modifies its
>>> contents. For paths of the above forms, musl's dirname returns a string
>>> literal "/" which shouldn't be modified.
>>>
>>> See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c
>>>
>>> There are a few other occurrences of the code shown above, but I've not
>>> checked to see if they could be made to segfault.
>>
>> Does this fix the problem?
>>
>> /* Bypass leading non-'/'s and then subsequent '/'s 
>> */
>> while (*s) {
>> if (*s == '/') {
>> do {
>> ++s;
>> } while (*s == '/');
>> c = *s; /* Save the current char */
>> added line==>   if (c)
>> *s = '\0'; /* and
>> replace it with nul */
>> break;
>
> This does prevent the segfault, but I'm not sure if depending on being able to
> modify the result of dirname() is reliable.

https://git.busybox.net/busybox/commit/?id=cf2600c3661c11491a838ef29733583afb6ad968

There are other occurrences of dirname + bb_make_directory that may have
this issue.

The following also segfaults.

$ cp --parents a /
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: segfault bb_make_directory + dirname with musl

[Guillermo Rodriguez Garcia]

2016-12-02 8:48 GMT+01:00 Guillermo Rodriguez Garcia
:
> 2016-12-01 21:13 GMT+01:00 Daniel Sabogal :
>> On Wed, Nov 30, 2016 at 5:52 PM, Denys Vlasenko
>>  wrote:
[...]
>>
>> This does prevent the segfault, but I'm not sure if depending on being able 
>> to
>> modify the result of dirname() is reliable.

(sorry for the previous empty mail)

I don't think it is reliable. The manpage for dirname says (emphasis mine):

   These  functions  may  return  pointers  to statically allocated memory
   which may be overwritten by subsequent calls.  Alternatively, they  may
   return  a  pointer to some part of path, so that *the string referred to
   by path should not be modified or freed* until the pointer  returned  by
   the function is no longer required.

Best,

Guillermo Rodriguez Garcia
guille.rodrig...@gmail.com
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: segfault bb_make_directory + dirname with musl

[Guillermo Rodriguez Garcia]

2016-12-01 21:13 GMT+01:00 Daniel Sabogal :
> On Wed, Nov 30, 2016 at 5:52 PM, Denys Vlasenko
>  wrote:
>> On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  wrote:
>>> The following commands cause busybox to segfault on musl-based systems.
>>>
>>> $ install -D a /
>>> $ install -D a /b
>>> $ install -D a /b/
>>>
>>> This happens because the code in
>>>
>>> https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196
>>>
>>> passes the result of dirname() to bb_make_directory() which modifies its
>>> contents. For paths of the above forms, musl's dirname returns a string
>>> literal "/" which shouldn't be modified.
>>>
>>> See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c
>>>
>>> There are a few other occurrences of the code shown above, but I've not
>>> checked to see if they could be made to segfault.
>>
>> Does this fix the problem?
>>
>> /* Bypass leading non-'/'s and then subsequent '/'s 
>> */
>> while (*s) {
>> if (*s == '/') {
>> do {
>> ++s;
>> } while (*s == '/');
>> c = *s; /* Save the current char */
>> added line==>   if (c)
>> *s = '\0'; /* and
>> replace it with nul */
>> break;
>
> This does prevent the segfault, but I'm not sure if depending on being able to
> modify the result of dirname() is reliable.
> ___
> busybox mailing list
> busybox@busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox



-- 
Guillermo Rodriguez Garcia
guille.rodrig...@gmail.com
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: segfault bb_make_directory + dirname with musl

[Daniel Sabogal]

On Wed, Nov 30, 2016 at 5:52 PM, Denys Vlasenko
 wrote:
> On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  wrote:
>> The following commands cause busybox to segfault on musl-based systems.
>>
>> $ install -D a /
>> $ install -D a /b
>> $ install -D a /b/
>>
>> This happens because the code in
>>
>> https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196
>>
>> passes the result of dirname() to bb_make_directory() which modifies its
>> contents. For paths of the above forms, musl's dirname returns a string
>> literal "/" which shouldn't be modified.
>>
>> See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c
>>
>> There are a few other occurrences of the code shown above, but I've not
>> checked to see if they could be made to segfault.
>
> Does this fix the problem?
>
> /* Bypass leading non-'/'s and then subsequent '/'s */
> while (*s) {
> if (*s == '/') {
> do {
> ++s;
> } while (*s == '/');
> c = *s; /* Save the current char */
> added line==>   if (c)
> *s = '\0'; /* and
> replace it with nul */
> break;

This does prevent the segfault, but I'm not sure if depending on being able to
modify the result of dirname() is reliable.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: segfault bb_make_directory + dirname with musl

[Denys Vlasenko]

On Wed, Nov 30, 2016 at 3:46 AM, Daniel Sabogal  wrote:
> The following commands cause busybox to segfault on musl-based systems.
>
> $ install -D a /
> $ install -D a /b
> $ install -D a /b/
>
> This happens because the code in
>
> https://git.busybox.net/busybox/tree/coreutils/install.c?h=1_25_1#n196
>
> passes the result of dirname() to bb_make_directory() which modifies its
> contents. For paths of the above forms, musl's dirname returns a string
> literal "/" which shouldn't be modified.
>
> See http://git.musl-libc.org/cgit/musl/tree/src/misc/dirname.c
>
> There are a few other occurrences of the code shown above, but I've not
> checked to see if they could be made to segfault.

Does this fix the problem?

/* Bypass leading non-'/'s and then subsequent '/'s */
while (*s) {
if (*s == '/') {
do {
++s;
} while (*s == '/');
c = *s; /* Save the current char */
added line==>   if (c)
*s = '\0'; /* and
replace it with nul */
break;
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox