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

Message ID 20200806143209.4044-1-nixiaoming@huawei.com
State New
Headers show
Series
  • stdlib: realpath use malloc replace __alloca to reduce stack overflow risks [BZ #26341]
Related show

Commit Message

Xiaoming Ni Aug. 6, 2020, 2:32 p.m.
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(-)

-- 
2.27.0

Comments

Vitaly Buka via Libc-alpha Aug. 6, 2020, 7:24 p.m. | #1
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)

>

Patch

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)