Fix getcwd03 testcase by zeroing target buffer for readlink()

PATCH New Helge Deller 2 participants 4 messages May 4, 2017 – May 9, 2017 Library code

Share & tools

Helge Deller

author

2017-05-04 18:23 UTC #0 lore raw

PATCH 1/1 Fix getcwd03 testcase by zeroing target buffer for readlink()

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 <[email protected]>

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

testcases/kernel/syscalls/getcwd/getcwd03.c +1 -0

index 4f8f872cf..ec1cacea9 100644
@@ -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))) {

Cyril Hrubis

2017-05-05 09:34 UTC #1 lore raw

PATCH 1/1 Fix getcwd03 testcase by zeroing target buffer for readlink()

Hi!

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 22 lines

Signed-off-by Helge Deller <[email protected]>

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

testcases/kernel/syscalls/getcwd/getcwd03.c +1 -0

index 4f8f872cf..ec1cacea9 100644
@@ -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))) {

Signature · 2 lines

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Signature · 2 lines

Cyril Hrubis
[email protected]

Helge Deller

author

2017-05-06 07:00 UTC #2 lore raw

PATCH 1/1 Fix getcwd03 testcase by zeroing target buffer for readlink()

* Cyril Hrubis <[email protected]>:

Quoted 8 lines

Hi!
Quoted 4 lines
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.

Quoted 7 lines

Quoted 2 lines
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.

Quoted 4 lines

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

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 <[email protected]>

lib/safe_macros.c +8 -0

index bffc5a17a..1267b0098 100644
@@ -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;

Cyril Hrubis

2017-05-09 09:08 UTC #3 lore raw

PATCH 1/1 Fix getcwd03 testcase by zeroing target buffer for readlink()

Hi!

Quoted 18 lines

Quoted 7 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 2 lines
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:

Quoted 7 lines

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.

Quoted 10 lines

Quoted 4 lines
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.

Quoted 24 lines

----------------
[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 <[email protected]>
lib/safe_macros.c +0 -0
index bffc5a17a..1267b0098 100644

lib/safe_macros.c ssize_t safe_readlink(const char *file, const int lineno, · line 411

Quoted 4 lines

lib/safe_macros.c +1 -0

@@ -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.

lib/safe_macros.c ssize_t safe_readlink(const char *file, const int lineno, · line 421

Quoted 10 lines

lib/safe_macros.c +7 -0

 		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.

Quoted 3 lines

lib/safe_macros.c +0 -0
 	}
 
 	return rval;

Signature · 2 lines

Cyril Hrubis
[email protected]