[LTP] [PATCH] Fix getcwd03 testcase by zeroing target buffer for readlink()

[LTP] [PATCH] Fix getcwd03 testcase by zeroing target buffer for readlink()

[Helge Deller]

According to the man(2) page of readlink(), a null byte is not appended to the
target buffer. So applications need to make sure that the target buffer is
zero-initialized, otherwise random bytes at the end of the returned string may
exist. 

This patch zero-initializes the on-stack char array "link" and thus fixes the
testcase failure of getcwd03 on the hppa/parisc architecture (and maybe others).

Signed-off-by: Helge Deller <deller@gmx.de>

--
 testcases/kernel/syscalls/getcwd/getcwd03.c |    1 +
 1 file changed, 1 insertion(+)


diff --git a/testcases/kernel/syscalls/getcwd/getcwd03.c b/testcases/kernel/syscalls/getcwd/getcwd03.c
index 4f8f872cf..ec1cacea9 100644
--- a/testcases/kernel/syscalls/getcwd/getcwd03.c
+++ b/testcases/kernel/syscalls/getcwd/getcwd03.c
@@ -74,6 +74,7 @@ static void verify_getcwd(void)
 	}
 
 	SAFE_CHDIR("..");
+	memset(link, 0, sizeof(link));
 	SAFE_READLINK(dir_link, link, sizeof(link));
 
 	if (strcmp(link, SAFE_BASENAME(res1))) {

[LTP] [PATCH] Fix getcwd03 testcase by zeroing target buffer for readlink()

[Cyril Hrubis]

Hi!
> According to the man(2) page of readlink(), a null byte is not appended to the
> target buffer. So applications need to make sure that the target buffer is
> zero-initialized, otherwise random bytes at the end of the returned string may
> exist. 

Good catch.

> This patch zero-initializes the on-stack char array "link" and thus fixes the
> testcase failure of getcwd03 on the hppa/parisc architecture (and maybe others).

Can we use the return value form the readlink instead?

	r = SAFE_READLINK(.., buf, sizeof(buf)-1);
	buf[r] = '\0';

The SAFE_READLINK() does catch the case where readlink() returns -1 and
passing size decreased by one should handle the corner case where the
path is truncated and return value is the size of the buffer we passed
to the readlink() call.

> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> --
>  testcases/kernel/syscalls/getcwd/getcwd03.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> 
> diff --git a/testcases/kernel/syscalls/getcwd/getcwd03.c b/testcases/kernel/syscalls/getcwd/getcwd03.c
> index 4f8f872cf..ec1cacea9 100644
> --- a/testcases/kernel/syscalls/getcwd/getcwd03.c
> +++ b/testcases/kernel/syscalls/getcwd/getcwd03.c
> @@ -74,6 +74,7 @@ static void verify_getcwd(void)
>  	}
>  
>  	SAFE_CHDIR("..");
> +	memset(link, 0, sizeof(link));
>  	SAFE_READLINK(dir_link, link, sizeof(link));
>  
>  	if (strcmp(link, SAFE_BASENAME(res1))) {
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] Fix getcwd03 testcase by zeroing target buffer for readlink()

[Helge Deller]

* Cyril Hrubis <chrubis@suse.cz>:
> Hi!
> > According to the man(2) page of readlink(), a null byte is not appended to the
> > target buffer. So applications need to make sure that the target buffer is
> > zero-initialized, otherwise random bytes at the end of the returned string may
> > exist. 
> 
> Good catch.
> 
> > This patch zero-initializes the on-stack char array "link" and thus fixes the
> > testcase failure of getcwd03 on the hppa/parisc architecture (and maybe others).
> 
> Can we use the return value form the readlink instead?
> 
> 	r = SAFE_READLINK(.., buf, sizeof(buf)-1);
> 	buf[r] = '\0';

Yes, that's possible.
But we have in total three testcase which exhibit the same problem:

testcases/kernel/syscalls/openat/openat03.c:    SAFE_READLINK(cleanup, path, tmp, PATH_MAX);
testcases/kernel/syscalls/getcwd/getcwd03.c:    SAFE_READLINK(dir_link, link, sizeof(link));
testcases/kernel/syscalls/open/open14.c:        SAFE_READLINK(cleanup, path, tmp, PATH_MAX);

All of those use a temporary un-initialized buffer on the stack, so all
would need this fix. On the hppa architecture I found those testcases
failing randomly.

> The SAFE_READLINK() does catch the case where readlink() returns -1 and
> passing size decreased by one should handle the corner case where the
> path is truncated and return value is the size of the buffer we passed
> to the readlink() call.

This still doesn't zero-terminate the buffer (which is on stack).

Instead I'd propose the patch below. It always zero-terminates the
buffer and as such we would handle newly added testcases in future which
will probably miss this readlink behaviour too.

Helge

----------------

[PATCH] SAFE_READLINK() should return zero-terminated target buffer

According to the man(2) page of readlink(), a null byte is not appended
to the target buffer. So applications need to make sure that the target
buffer is zero-initialized, otherwise random bytes at the end of the
returned string may exist.

Currently all users of SAFE_READLINK() get it wrong. This include the
openat03, getcwd03 and open14 testcases which pass a temporary,
un-initialized on-stack buffer to readlink().

This patch fixes SAFE_READLINK to always return a zero-terminated string
and thus fixes the the failure of getcwd03 on the hppa/parisc
architecture (and probably others).

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/lib/safe_macros.c b/lib/safe_macros.c
index bffc5a17a..1267b0098 100644
--- a/lib/safe_macros.c
+++ b/lib/safe_macros.c
@@ -408,9 +408,17 @@ ssize_t safe_readlink(const char *file, const int lineno,
 	rval = readlink(path, buf, bufsize);
 
 	if (rval == -1) {
+		buf[0] = 0; /* clear buffer on error */
 		tst_brkm(TBROK | TERRNO, cleanup_fn,
 			 "%s:%d: readlink(%s,%p,%zu) failed",
 			 file, lineno, path, buf, bufsize);
+	} else {
+		/* readlink does not append a null byte to the buffer.
+		 * Add it now. */
+		if ((size_t) rval < bufsize)
+			buf[rval] = 0;
+		else
+			buf[bufsize-1] = 0;
 	}
 
 	return rval;

[LTP] [PATCH] Fix getcwd03 testcase by zeroing target buffer for readlink()

[Cyril Hrubis]

Hi!
> > > According to the man(2) page of readlink(), a null byte is not appended to the
> > > target buffer. So applications need to make sure that the target buffer is
> > > zero-initialized, otherwise random bytes at the end of the returned string may
> > > exist. 
> > 
> > Good catch.
> > 
> > > This patch zero-initializes the on-stack char array "link" and thus fixes the
> > > testcase failure of getcwd03 on the hppa/parisc architecture (and maybe others).
> > 
> > Can we use the return value form the readlink instead?
> > 
> > 	r = SAFE_READLINK(.., buf, sizeof(buf)-1);
> > 	buf[r] = '\0';
> 
> Yes, that's possible.
> But we have in total three testcase which exhibit the same problem:
> 
> testcases/kernel/syscalls/openat/openat03.c:    SAFE_READLINK(cleanup, path, tmp, PATH_MAX);
> testcases/kernel/syscalls/getcwd/getcwd03.c:    SAFE_READLINK(dir_link, link, sizeof(link));
> testcases/kernel/syscalls/open/open14.c:        SAFE_READLINK(cleanup, path, tmp, PATH_MAX);
> 
> All of those use a temporary un-initialized buffer on the stack, so all
> would need this fix. On the hppa architecture I found those testcases
> failing randomly.

Ok.

> > The SAFE_READLINK() does catch the case where readlink() returns -1 and
> > passing size decreased by one should handle the corner case where the
> > path is truncated and return value is the size of the buffer we passed
> > to the readlink() call.
> 
> This still doesn't zero-terminate the buffer (which is on stack).
> 
> Instead I'd propose the patch below. It always zero-terminates the
> buffer and as such we would handle newly added testcases in future which
> will probably miss this readlink behaviour too.

Fixing the problem in SAFE_READLIN() is a good idea, comments below.

> ----------------
> 
> [PATCH] SAFE_READLINK() should return zero-terminated target buffer
> 
> According to the man(2) page of readlink(), a null byte is not appended
> to the target buffer. So applications need to make sure that the target
> buffer is zero-initialized, otherwise random bytes at the end of the
> returned string may exist.
> 
> Currently all users of SAFE_READLINK() get it wrong. This include the
> openat03, getcwd03 and open14 testcases which pass a temporary,
> un-initialized on-stack buffer to readlink().
> 
> This patch fixes SAFE_READLINK to always return a zero-terminated string
> and thus fixes the the failure of getcwd03 on the hppa/parisc
> architecture (and probably others).
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/lib/safe_macros.c b/lib/safe_macros.c
> index bffc5a17a..1267b0098 100644
> --- a/lib/safe_macros.c
> +++ b/lib/safe_macros.c
> @@ -408,9 +408,17 @@ ssize_t safe_readlink(const char *file, const int lineno,
>  	rval = readlink(path, buf, bufsize);
>  
>  	if (rval == -1) {
> +		buf[0] = 0; /* clear buffer on error */

Clearing the buffer here is not really needed since the tst_brkm() will
abort the test anyway.

>  		tst_brkm(TBROK | TERRNO, cleanup_fn,
>  			 "%s:%d: readlink(%s,%p,%zu) failed",
>  			 file, lineno, path, buf, bufsize);
> +	} else {
> +		/* readlink does not append a null byte to the buffer.
> +		 * Add it now. */
> +		if ((size_t) rval < bufsize)
> +			buf[rval] = 0;
> +		else
> +			buf[bufsize-1] = 0;

This looks good.

>  	}
>  
>  	return rval;

-- 
Cyril Hrubis
chrubis@suse.cz