[PATCH] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]

[PATCH] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]

[Xiaoming Ni]

Realpath() cyclically invokes __alloca() when processing soft link files,
which may consume 164 KB stack space.
Therefore, replace __alloca with malloc to reduce stack overflow risks

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 stdlib/canonicalize.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
index cbd885a3c5..d3130d81f0 100644
--- a/stdlib/canonicalize.c
+++ b/stdlib/canonicalize.c
@@ -163,27 +163,46 @@ __realpath (const char *name, char *resolved)
 
 	  if (S_ISLNK (st.st_mode))
 	    {
-	      char *buf = __alloca (path_max);
+	      char *buf = malloc (path_max);
 	      size_t len;
 
+	      if (!buf)
+	        {
+	          __set_errno (ENOMEM);
+	          goto error;
+	        }
+
 	      if (++num_links > __eloop_threshold ())
 		{
 		  __set_errno (ELOOP);
+		  free(buf);
 		  goto error;
 		}
 
 	      n = __readlink (rpath, buf, path_max - 1);
 	      if (n < 0)
-		goto error;
+	        {
+	          free(buf);
+	          goto error;
+	        }
 	      buf[n] = '\0';
 
 	      if (!extra_buf)
-		extra_buf = __alloca (path_max);
+	        {
+	          extra_buf = malloc (path_max);
+	          if (!extra_buf)
+	            {
+	              free(buf);
+	              __set_errno (ENOMEM);
+	              goto error;
+	            }
+	        }
 
 	      len = strlen (end);
 	      if (path_max - n <= len)
 		{
 		  __set_errno (ENAMETOOLONG);
+		  free(buf);
 		  goto error;
 		}
 
@@ -197,6 +216,7 @@ __realpath (const char *name, char *resolved)
 		/* Back up to previous component, ignore if at root already: */
 		if (dest > rpath + 1)
 		  while ((--dest)[-1] != '/');
+	      free(buf);
 	    }
 	  else if (!S_ISDIR (st.st_mode) && *end != '\0')
 	    {
@@ -210,12 +230,14 @@ __realpath (const char *name, char *resolved)
   *dest = '\0';
 
   assert (resolved == NULL || resolved == rpath);
+  free(extra_buf);
   return rpath;
 
 error:
   assert (resolved == NULL || resolved == rpath);
   if (resolved == NULL)
     free (rpath);
+  free(extra_buf);
   return NULL;
 }
 libc_hidden_def (__realpath)
-- 
2.27.0

Re: [PATCH] stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]

[Adhemerval Zanella]

On 06/08/2020 11:32, Xiaoming Ni wrote:
> Realpath() cyclically invokes __alloca() when processing soft link files,
> which may consume 164 KB stack space.
> Therefore, replace __alloca with malloc to reduce stack overflow risks
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>

We do not use SCO, but rather copyright assignments (Carlos can check if your 
is ok with FSF).

If possible could you provide a testcase? Maybe by limiting the stack with
getrlimit and using the example provided in the bugzilla?  Patch look ok, 
just formatting style nits. 

As a side note, for Linux we could simplify the realpath implementation
a *lot* with limited stack size and no malloc allocation we could remove
the GNU extension to return prefix of path that is not readable or does 
not exist if resolved_path is not NULL (is a GNU extension that came
from the implementation itself that works directly on the buffer).


> ---
>  stdlib/canonicalize.c | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/stdlib/canonicalize.c b/stdlib/canonicalize.c
> index cbd885a3c5..d3130d81f0 100644
> --- a/stdlib/canonicalize.c
> +++ b/stdlib/canonicalize.c
> @@ -163,27 +163,46 @@ __realpath (const char *name, char *resolved)
>  
>  	  if (S_ISLNK (st.st_mode))
>  	    {
> -	      char *buf = __alloca (path_max);
> +	      char *buf = malloc (path_max);
>  	      size_t len;
>  
> +	      if (!buf)

Use explicit check (buf != NULL).

> +	        {
> +	          __set_errno (ENOMEM);
> +	          goto error;
> +	        }
> +
>  	      if (++num_links > __eloop_threshold ())
>  		{
>  		  __set_errno (ELOOP);
> +		  free(buf);

Space after free.

>  		  goto error;
>  		}
>  
>  	      n = __readlink (rpath, buf, path_max - 1);
>  	      if (n < 0)
> -		goto error;
> +	        {
> +	          free(buf);

Ditto.

> +	          goto error;
> +	        }
>  	      buf[n] = '\0';
>  
>  	      if (!extra_buf)
> -		extra_buf = __alloca (path_max);
> +	        {
> +	          extra_buf = malloc (path_max);
> +	          if (!extra_buf)

Use explicit check (extra_buf != NULL).

> +	            {
> +	              free(buf);

Space after free.

> +	              __set_errno (ENOMEM);
> +	              goto error;
> +	            }
> +	        }
>  
>  	      len = strlen (end);
>  	      if (path_max - n <= len)
>  		{
>  		  __set_errno (ENAMETOOLONG);
> +		  free(buf);

Ditto.

>  		  goto error;
>  		}
>  
> @@ -197,6 +216,7 @@ __realpath (const char *name, char *resolved)
>  		/* Back up to previous component, ignore if at root already: */
>  		if (dest > rpath + 1)
>  		  while ((--dest)[-1] != '/');
> +	      free(buf);

Ditto.

>  	    }
>  	  else if (!S_ISDIR (st.st_mode) && *end != '\0')
>  	    {
> @@ -210,12 +230,14 @@ __realpath (const char *name, char *resolved)
>    *dest = '\0';
>  
>    assert (resolved == NULL || resolved == rpath);
> +  free(extra_buf);
>    return rpath;
>  
>  error:
>    assert (resolved == NULL || resolved == rpath);
>    if (resolved == NULL)
>      free (rpath);
> +  free(extra_buf);

Space after free.

>    return NULL;
>  }
>  libc_hidden_def (__realpath)
>