getmntent: Generalize octal decoding

Message ID 20201216142415.173885-1-siddhesh@sourceware.org
State Deferred
Headers
Series getmntent: Generalize octal decoding |

Commit Message

Siddhesh Poyarekar Dec. 16, 2020, 2:24 p.m. UTC
  The current octal decoding looks for specific characters to decode.
generalize this instead, allowing any arbitrary \xxx octal pattern to
be decoded as long as it is a printable character.

The decoding is now done a bit later so that it does not influence the
"ignore" option of autofs, which is a hint for getmntent to skip over
the line.  With this change, "\151gnore" will not cuase the line to be
skipped over, but the options string will show "ignore" correctly.

Also add a test case to validate that encoded hashes get decoded
correctly.  This would be useful if/when the kernel starts escaping
hashes in /proc/mounts.
---
 misc/mntent_r.c          | 92 ++++++++++++++++++++++++----------------
 misc/tst-mntent-autofs.c |  4 +-
 2 files changed, 58 insertions(+), 38 deletions(-)
  

Comments

Siddhesh Poyarekar Dec. 22, 2020, 7:48 a.m. UTC | #1
On 12/16/20 7:54 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> The current octal decoding looks for specific characters to decode.
> generalize this instead, allowing any arbitrary \xxx octal pattern to
> be decoded as long as it is a printable character.
> 
> The decoding is now done a bit later so that it does not influence the
> "ignore" option of autofs, which is a hint for getmntent to skip over
> the line.  With this change, "\151gnore" will not cuase the line to be
> skipped over, but the options string will show "ignore" correctly.
> 
> Also add a test case to validate that encoded hashes get decoded
> correctly.  This would be useful if/when the kernel starts escaping
> hashes in /proc/mounts.
> ---
>   misc/mntent_r.c          | 92 ++++++++++++++++++++++++----------------
>   misc/tst-mntent-autofs.c |  4 +-
>   2 files changed, 58 insertions(+), 38 deletions(-)
> 
> diff --git a/misc/mntent_r.c b/misc/mntent_r.c
> index b50ba0de89..ac56e40fd5 100644
> --- a/misc/mntent_r.c
> +++ b/misc/mntent_r.c
> @@ -65,6 +65,27 @@ __endmntent (FILE *stream)
>   libc_hidden_def (__endmntent)
>   weak_alias (__endmntent, endmntent)
>   
> +static char
> +parse_octal (char *s)
> +{
> +  char c = 0, t;
> +
> +  t = s[0] - '0';
> +  if (t & (~0x7))
> +    return 0;
> +  c |= t << 6;
> +
> +  t = s[1] - '0';
> +  if (t & (~0x7))
> +    return 0;
> +  c |= t << 3;
> +
> +  t = s[2] - '0';
> +  if (t & (~0x7))
> +    return 0;
> +
> +  return c | t;
> +}
>   
>   /* Since the values in a line are separated by spaces, a name cannot
>      contain a space.  Therefore some programs encode spaces in names
> @@ -77,43 +98,42 @@ decode_name (char *buf)
>     char *wp = buf;
>   
>     do
> -    if (rp[0] == '\\' && rp[1] == '0' && rp[2] == '4' && rp[3] == '0')
> -      {
> -	/* \040 is a SPACE.  */
> -	*wp++ = ' ';
> -	rp += 3;
> -      }
> -    else if (rp[0] == '\\' && rp[1] == '0' && rp[2] == '1' && rp[3] == '1')
> -      {
> -	/* \011 is a TAB.  */
> -	*wp++ = '\t';
> -	rp += 3;
> -      }
> -    else if (rp[0] == '\\' && rp[1] == '0' && rp[2] == '1' && rp[3] == '2')
> -      {
> -	/* \012 is a NEWLINE.  */
> -	*wp++ = '\n';
> -	rp += 3;
> -      }
> -    else if (rp[0] == '\\' && rp[1] == '\\')
> -      {
> -	/* We have to escape \\ to be able to represent all characters.  */
> -	*wp++ = '\\';
> -	rp += 1;
> -      }
> -    else if (rp[0] == '\\' && rp[1] == '1' && rp[2] == '3' && rp[3] == '4')
> -      {
> -	/* \134 is also \\.  */
> -	*wp++ = '\\';
> -	rp += 3;
> -      }
> -    else
> +    {
> +      if (rp[0] == '\\')
> +	{
> +	  char c;
> +
> +	  if ((c = parse_octal (&rp[1])) != 0)
> +	    {
> +	      *wp++ = c;
> +	      rp += 3;
> +	      continue;
> +	    }
> +	  if (rp[1] == '\\')
> +	    {
> +	      *wp++ = '\\';
> +	      rp++;
> +	      continue;
> +	    }
> +	}
>         *wp++ = *rp;
> +    }
>     while (*rp++ != '\0');
>   
>     return buf;
>   }
>   
> +static struct mntent *
> +decode_mntent_names (struct mntent *res)
> +{
> +  res->mnt_fsname = decode_name (res->mnt_fsname);
> +  res->mnt_dir = decode_name (res->mnt_dir);
> +  res->mnt_type = decode_name (res->mnt_type);
> +  res->mnt_opts = decode_name (res->mnt_opts);
> +
> +  return res;
> +}
> +
>   static bool
>   get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
>   {
> @@ -151,19 +171,19 @@ get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
>     while (head[0] == '\0' || head[0] == '#');
>   
>     cp = __strsep (&head, " \t");
> -  mp->mnt_fsname = cp != NULL ? decode_name (cp) : (char *) "";
> +  mp->mnt_fsname = cp != NULL ? cp : (char *) "";
>     if (head)
>       head += strspn (head, " \t");
>     cp = __strsep (&head, " \t");
> -  mp->mnt_dir = cp != NULL ? decode_name (cp) : (char *) "";
> +  mp->mnt_dir = cp != NULL ? cp : (char *) "";
>     if (head)
>       head += strspn (head, " \t");
>     cp = __strsep (&head, " \t");
> -  mp->mnt_type = cp != NULL ? decode_name (cp) : (char *) "";
> +  mp->mnt_type = cp != NULL ? cp : (char *) "";
>     if (head)
>       head += strspn (head, " \t");
>     cp = __strsep (&head, " \t");
> -  mp->mnt_opts = cp != NULL ? decode_name (cp) : (char *) "";
> +  mp->mnt_opts = cp != NULL ? cp : (char *) "";
>     switch (head ? sscanf (head, " %d %d ", &mp->mnt_freq, &mp->mnt_passno) : 0)
>       {
>       case 0:
> @@ -197,7 +217,7 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
>   	  memset (mp, 0, sizeof (*mp));
>   	else
>   	  {
> -	    result = mp;
> +	    result = decode_mntent_names (mp);
>   	    break;
>   	  }
>         }
> diff --git a/misc/tst-mntent-autofs.c b/misc/tst-mntent-autofs.c
> index e4c509f520..54b11b23ba 100644
> --- a/misc/tst-mntent-autofs.c
> +++ b/misc/tst-mntent-autofs.c
> @@ -87,8 +87,8 @@ static struct test_case test_cases[] =
>       /* These are not filtered because the string is escaped.  '\151'
>          is 'i', but it is not actually decoded by the parser.  */
>       { "/etc/auto.\\151gnore /mnt/auto/16 autofs \\151gnore 0 0",
> -      { "/etc/auto.\\151gnore", "/mnt/auto/16", "autofs",
> -        "\\151gnore", } },
> +      { "/etc/auto.ignore", "/mnt/auto/16", "autofs",
> +        "ignore", } },
>     };
>   
>   static int
>
  

Patch

diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index b50ba0de89..ac56e40fd5 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -65,6 +65,27 @@  __endmntent (FILE *stream)
 libc_hidden_def (__endmntent)
 weak_alias (__endmntent, endmntent)
 
+static char
+parse_octal (char *s)
+{
+  char c = 0, t;
+
+  t = s[0] - '0';
+  if (t & (~0x7))
+    return 0;
+  c |= t << 6;
+
+  t = s[1] - '0';
+  if (t & (~0x7))
+    return 0;
+  c |= t << 3;
+
+  t = s[2] - '0';
+  if (t & (~0x7))
+    return 0;
+
+  return c | t;
+}
 
 /* Since the values in a line are separated by spaces, a name cannot
    contain a space.  Therefore some programs encode spaces in names
@@ -77,43 +98,42 @@  decode_name (char *buf)
   char *wp = buf;
 
   do
-    if (rp[0] == '\\' && rp[1] == '0' && rp[2] == '4' && rp[3] == '0')
-      {
-	/* \040 is a SPACE.  */
-	*wp++ = ' ';
-	rp += 3;
-      }
-    else if (rp[0] == '\\' && rp[1] == '0' && rp[2] == '1' && rp[3] == '1')
-      {
-	/* \011 is a TAB.  */
-	*wp++ = '\t';
-	rp += 3;
-      }
-    else if (rp[0] == '\\' && rp[1] == '0' && rp[2] == '1' && rp[3] == '2')
-      {
-	/* \012 is a NEWLINE.  */
-	*wp++ = '\n';
-	rp += 3;
-      }
-    else if (rp[0] == '\\' && rp[1] == '\\')
-      {
-	/* We have to escape \\ to be able to represent all characters.  */
-	*wp++ = '\\';
-	rp += 1;
-      }
-    else if (rp[0] == '\\' && rp[1] == '1' && rp[2] == '3' && rp[3] == '4')
-      {
-	/* \134 is also \\.  */
-	*wp++ = '\\';
-	rp += 3;
-      }
-    else
+    {
+      if (rp[0] == '\\')
+	{
+	  char c;
+
+	  if ((c = parse_octal (&rp[1])) != 0)
+	    {
+	      *wp++ = c;
+	      rp += 3;
+	      continue;
+	    }
+	  if (rp[1] == '\\')
+	    {
+	      *wp++ = '\\';
+	      rp++;
+	      continue;
+	    }
+	}
       *wp++ = *rp;
+    }
   while (*rp++ != '\0');
 
   return buf;
 }
 
+static struct mntent *
+decode_mntent_names (struct mntent *res)
+{
+  res->mnt_fsname = decode_name (res->mnt_fsname);
+  res->mnt_dir = decode_name (res->mnt_dir);
+  res->mnt_type = decode_name (res->mnt_type);
+  res->mnt_opts = decode_name (res->mnt_opts);
+
+  return res;
+}
+
 static bool
 get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 {
@@ -151,19 +171,19 @@  get_mnt_entry (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
   while (head[0] == '\0' || head[0] == '#');
 
   cp = __strsep (&head, " \t");
-  mp->mnt_fsname = cp != NULL ? decode_name (cp) : (char *) "";
+  mp->mnt_fsname = cp != NULL ? cp : (char *) "";
   if (head)
     head += strspn (head, " \t");
   cp = __strsep (&head, " \t");
-  mp->mnt_dir = cp != NULL ? decode_name (cp) : (char *) "";
+  mp->mnt_dir = cp != NULL ? cp : (char *) "";
   if (head)
     head += strspn (head, " \t");
   cp = __strsep (&head, " \t");
-  mp->mnt_type = cp != NULL ? decode_name (cp) : (char *) "";
+  mp->mnt_type = cp != NULL ? cp : (char *) "";
   if (head)
     head += strspn (head, " \t");
   cp = __strsep (&head, " \t");
-  mp->mnt_opts = cp != NULL ? decode_name (cp) : (char *) "";
+  mp->mnt_opts = cp != NULL ? cp : (char *) "";
   switch (head ? sscanf (head, " %d %d ", &mp->mnt_freq, &mp->mnt_passno) : 0)
     {
     case 0:
@@ -197,7 +217,7 @@  __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 	  memset (mp, 0, sizeof (*mp));
 	else
 	  {
-	    result = mp;
+	    result = decode_mntent_names (mp);
 	    break;
 	  }
       }
diff --git a/misc/tst-mntent-autofs.c b/misc/tst-mntent-autofs.c
index e4c509f520..54b11b23ba 100644
--- a/misc/tst-mntent-autofs.c
+++ b/misc/tst-mntent-autofs.c
@@ -87,8 +87,8 @@  static struct test_case test_cases[] =
     /* These are not filtered because the string is escaped.  '\151'
        is 'i', but it is not actually decoded by the parser.  */
     { "/etc/auto.\\151gnore /mnt/auto/16 autofs \\151gnore 0 0",
-      { "/etc/auto.\\151gnore", "/mnt/auto/16", "autofs",
-        "\\151gnore", } },
+      { "/etc/auto.ignore", "/mnt/auto/16", "autofs",
+        "ignore", } },
   };
 
 static int