klibc - Jun 2011 - [PATCH 0/2] Correct various strndup() problems

The current implementation of strndup() has some shortcomings that can
lead to a fatal error.

 - If we pass a maximum string length larger than the copied length, we
   will corrupt some data beyond the end of the newly allocated buffer.

 - The maximum length does not prevent access to memory beyond the
   maximum length, which can lead to unexpectd errors with strings not
   terminated by 0.

Romain Izard (2):
  strndup(): Fix out of bounds read access
  strndup(): Do not corrupt the memory pool

 usr/klibc/strndup.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

The use of strlen to get the length of the source string can lead to
undetermined memory access if the source string is not finished with a
zero. Use strnlen to prevent this.

Signed-off-by: Romain Izard <romain.izard.pro at gmail.com>
---
 usr/klibc/strndup.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/usr/klibc/strndup.c b/usr/klibc/strndup.c
index 65afd44..e4814be 100644
--- a/usr/klibc/strndup.c
+++ b/usr/klibc/strndup.c
@@ -7,9 +7,8 @@
 
 char *strndup(const char *s, size_t n)
 {
-	int l = n > strlen(s) ? strlen(s) + 1 : n + 1;
-	char *d = malloc(l);
-
+	size_t l = strnlen(s, n);
+	char *d = malloc(l + 1);
 	if (!d)
 		return NULL;
 
-- 
1.7.0.4

The allocated string may be shorter than the requested length. Always
use the shortest length to write the terminating zero

Signed-off-by: Romain Izard <romain.izard.pro at gmail.com>
---
 usr/klibc/strndup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/usr/klibc/strndup.c b/usr/klibc/strndup.c
index e4814be..20eaa8b 100644
--- a/usr/klibc/strndup.c
+++ b/usr/klibc/strndup.c
@@ -13,6 +13,6 @@ char *strndup(const char *s, size_t n)
 		return NULL;
 
 	memcpy(d, s, l);
-	d[n] = '\0';
+	d[l] = '\0';
 	return d;
 }
-- 
1.7.0.4

On 06/24/2011 02:01 AM, Romain Izard wrote:
> The current implementation of strndup() has some shortcomings that can
> lead to a fatal error.
> 
>  - If we pass a maximum string length larger than the copied length, we
>    will corrupt some data beyond the end of the newly allocated buffer.
> 
>  - The maximum length does not prevent access to memory beyond the
>    maximum length, which can lead to unexpectd errors with strings not
>    terminated by 0.
> 
> Romain Izard (2):
>   strndup(): Fix out of bounds read access
>   strndup(): Do not corrupt the memory pool
> 
>  usr/klibc/strndup.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
Looks great, thanks!

	-hpa

On Fri, 24 Jun 2011, H. Peter Anvin wrote:
> On 06/24/2011 02:01 AM, Romain Izard wrote:
> > The current implementation of strndup() has some shortcomings that can
> > lead to a fatal error.
> > 
> >  - If we pass a maximum string length larger than the copied length,
we
> >    will corrupt some data beyond the end of the newly allocated
buffer.
> > 
> >  - The maximum length does not prevent access to memory beyond the
> >    maximum length, which can lead to unexpectd errors with strings not
> >    terminated by 0.
> > 
> > Romain Izard (2):
> >   strndup(): Fix out of bounds read access
> >   strndup(): Do not corrupt the memory pool
> > 
> >  usr/klibc/strndup.c |    7 +++----
> >  1 files changed, 3 insertions(+), 4 deletions(-)
> > 
> 
> Looks great, thanks!
> 
applied and pushed, thank you!

-- 
maks