[PATCH] [BZ 17273] fix incorrect mount table entry parsing in __getmntent_r()

[PATCH] [BZ 17273] fix incorrect mount table entry parsing in __getmntent_r()

[Vladimir A. Nazarenko]

From: naszar <naszar@ya.ru>

When mount entry contains only four fields and have more
then one space or tab at the and, mp.mnt_freq and
mp.mnt_passno will be set to some specific values as side
effect from parsing of previus mount entry. It is because
sscanf(""," %d %d ", &a, &b) returns -1, but this case
is  unprocessed. Values of mp.mnt_freq and  mp.mnt_passno
stays unchanged. This patch is attempt to fix described issue
by removing trailing tabs and spaces.

	[BZ 17273]
	*misc/mntent_r.c: fix incorrect mount table entry parsing
---
 misc/mntent_r.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index e68ec8e..e0a0b9d 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -135,7 +135,11 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 
       end_ptr = strchr (buffer, '\n');
       if (end_ptr != NULL)	/* chop newline */
-	*end_ptr = '\0';
+	{
+	  while (end_ptr[-1] == ' ' || end_ptr[-1] == '\t')
+            end_ptr--;
+	  *end_ptr = '\0';
+	}
       else
 	{
 	  /* Not the whole line was read.  Do it now but forget it.  */
-- 
2.1.0

Re: [PATCH] [BZ 17273] fix incorrect mount table entry parsing in __getmntent_r()

[Roland McGrath]

Please write a test case.

> 	[BZ 17273]
> 	*misc/mntent_r.c: fix incorrect mount table entry parsing

This should look like:

	[BZ #17273]
	* misc/mntent_r.c (__getmntent_r): ...

and the actual text should be specific about how it changes the behavior.

Re: [PATCH] [BZ 17273] fix incorrect mount table entry parsing in __getmntent_r()

[Vladimir A. Nazarenko]

On 30.08.2014 08:58, Roland McGrath wrote:
> Please write a test case.
> 
Can I modify existing test or I must create new one? Is following patch OK?
Can I resend fixed patch and test case as *one* patch?
> 
> This should look like:
> 
how about that:

	[BZ #17273]
	* misc/mntent_r.c (__getmntent_r): cut off trailing spaces
	and tabs from bufer before parsing fstab entry.
	* misc/tst-mntent.c(main): add test for mount entry with 
	trailing spaces and tabs.

---
diff --git a/misc/tst-mntent.c b/misc/tst-mntent.c
index 802b56e..f568ba8 100644
--- a/misc/tst-mntent.c
+++ b/misc/tst-mntent.c
@@ -73,7 +73,21 @@ main (int argc, char *argv[])
 	  puts ("Error while reading written entry back in");
 	  result = 1;
 	}
-    }
+
+       /*part III: test if entry with trailing whitespaces*/
+      fputs("/foo\\040dir /bar\\040dir auto bind \t \n", fp);
+
+      rewind (fp);
+      
+      mnt = getmntent(fp);
+      mnt = getmntent(fp);
+      if (mnt->mnt_freq != 0 || mnt->mnt_passno != 0)
+	{
+	  printf("Error mnt_freq = %d and mnt_opts = %d, but zero expected\n",
+			  mnt->mnt_freq, mnt->mnt_passno);
+          result = 1;
+	}
+   }
 
   return result;
 }

Re: [PATCH] [BZ 17273] fix incorrect mount table entry parsing in __getmntent_r()

[Roland McGrath]

> On 30.08.2014 08:58, Roland McGrath wrote:
> > Please write a test case.
> > 
> Can I modify existing test or I must create new one? Is following patch OK?

If it is substantially simpler to modify an existing test, that is the
right thing to do.

> Can I resend fixed patch and test case as *one* patch?

Yes, that is always the right way to do it.  That way when you say that
you've run 'make check' and seen no regressions (on some particular
configuration, which you should cite--saying that and having it be true is
a necessary part of submitting any substantive change), that constitutes
testing for the newly-fixed case too.

> 	[BZ #17273]
> 	* misc/mntent_r.c (__getmntent_r): cut off trailing spaces
> 	and tabs from bufer before parsing fstab entry.

Capitalize sentences.

> 	* misc/tst-mntent.c(main): add test for mount entry with 
> 	trailing spaces and tabs.

Space before that paren.

> +       /*part III: test if entry with trailing whitespaces*/

Comments need proper grammar, capitalization, punctuation, and space usage.
Follow the existing examples.

> +      fputs("/foo\\040dir /bar\\040dir auto bind \t \n", fp);

Space before paren.

> +      mnt = getmntent(fp);
> +      mnt = getmntent(fp);

Space before paren.  Did you really mean to call it twice?

> +      if (mnt->mnt_freq != 0 || mnt->mnt_passno != 0)

Check all the fields like the existing case does.

> +	{
> +	  printf("Error mnt_freq = %d and mnt_opts = %d, but zero expected\n",
> +			  mnt->mnt_freq, mnt->mnt_passno);
> +          result = 1;
> +	}

Space before paren.  The second line of the block should be intended the
same as the first, and use a tab for the leading 8 spaces as the first line
does.

[PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Vladimir A. Nazarenko]

When mount entry contains only four fields and have more
then one space or tab at the and, mp.mnt_freq and
mp.mnt_passno will be set to some specific values as side
effect from parsing of previus mount entry. It is because
sscanf(""," %d %d ", &a, &b) returns -1, but this case
is  unprocessed. Values of mp.mnt_freq and  mp.mnt_passno
stays unchanged. This patch is attempt to fix described issue
by removing trailing tabs and spaces.

	[BZ #17273]
	* misc/mntent_r.c (__getmntent_r): Cut off trailing spaces
	and tabs from buffer before parsing fstab entry.
	* misc/tst-mntent.c (main): Add test for mount entry with
	trailing spaces and tabs.
---
 misc/mntent_r.c   |  6 +++++-
 misc/tst-mntent.c | 22 +++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index e68ec8e..e0a0b9d 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -135,7 +135,11 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 
       end_ptr = strchr (buffer, '\n');
       if (end_ptr != NULL)	/* chop newline */
-	*end_ptr = '\0';
+	{
+	  while (end_ptr[-1] == ' ' || end_ptr[-1] == '\t')
+            end_ptr--;
+	  *end_ptr = '\0';
+	}
       else
 	{
 	  /* Not the whole line was read.  Do it now but forget it.  */
diff --git a/misc/tst-mntent.c b/misc/tst-mntent.c
index 802b56e..876c89f 100644
--- a/misc/tst-mntent.c
+++ b/misc/tst-mntent.c
@@ -73,7 +73,27 @@ main (int argc, char *argv[])
 	  puts ("Error while reading written entry back in");
 	  result = 1;
 	}
-    }
+
+      /* Part III: Entry with whitespaces at the end of a line. */
+      rewind (fp);
+
+      fputs ("/foo\\040dir /bar\\040dir auto bind \t \n", fp);
+
+      rewind (fp);
+
+      mnt = getmntent (fp);
+
+      if (strcmp (mnt->mnt_fsname, "/foo dir") != 0
+	  || strcmp (mnt->mnt_dir, "/bar dir") != 0
+	  || strcmp (mnt->mnt_type, "auto") != 0
+	  || strcmp (mnt->mnt_opts, "bind") != 0
+	  || mnt->mnt_freq != 0
+	  || mnt->mnt_passno != 0)
+	{
+	  puts ("Error while reading entry with trailing whitespaces");
+	  result = 1;
+	}
+   }
 
   return result;
 }
-- 
2.1.1

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Roland McGrath]

The change looks fine to me now.  
I don't see copyright papers for you on file.

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Vladimir A. Nazarenko]

On 10.10.2014 05:21, Roland McGrath wrote:
> I don't see copyright papers for you on file.
> 
> .

I thought this changes are trivial. But anyway
say what I should do and I will do it. It's my 
first patch.
 

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Roland McGrath]

> I thought this changes are trivial. But anyway
> say what I should do and I will do it. It's my 
> first patch.

They are sort of borderline for the "number of lines of code" metric.  The
safe thing is to assume they do require copyright assignment.  If you are
going to make any more contributions to libc, you'll need an assignment on
file anyway.  I'll send you the details off-list.


Thanks,
Roland

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

On 04 Oct 2014 13:31, Vladimir A. Nazarenko wrote:
> --- a/misc/mntent_r.c
> +++ b/misc/mntent_r.c
> @@ -135,7 +135,11 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
>  
>        end_ptr = strchr (buffer, '\n');
>        if (end_ptr != NULL)	/* chop newline */
> -	*end_ptr = '\0';
> +	{
> +	  while (end_ptr[-1] == ' ' || end_ptr[-1] == '\t')
> +            end_ptr--;
> +	  *end_ptr = '\0';
> +	}

this randomly corrupts memory when you get a blank line which is pretty
common i think in /etc/fstab.  buffer = "\n" which means end_ptr will be
buffer which means end_ptr[-1] is random stack memory.  if it happens to
be 0x20 or 0x09, you corrupt a single byte.  happens whenever the line is
just whitespace as you walk back to the start of the buffer allocation.

the way the malloc heaps are laid out, it doesn't seem to be noticed for
most arches, but it's easily reproducible on ppc32.

https://sourceware.org/bugzilla/show_bug.cgi?id=18887
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

On Tue, Jan 6, 2015 at 4:04 PM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Wed, 7 Jan 2015, Vladimir A. Nazarenko wrote:
>
>> On 07.01.2015 03:11, H.J. Lu wrote:
>> >
>> > Has the copyright assignment issue been resolved?
>> >
>>
>> Yes, I think so. I received signed paper from FSF. Should I send it to
>> someone?
>
> I can confirm there is a copyright.list entry for Vladimir A. Nazarenko
> dated 2014-11-24, so no further action is needed there.
>

This is what I checked in.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-Fix-incorrect-mount-table-entry-parsing-in-__getmnte.patch --]
[-- Type: text/x-patch, Size: 3906 bytes --]

From fb87ee96d7dd0714d52004e4676629f8d9db732f Mon Sep 17 00:00:00 2001
From: "Vladimir A. Nazarenko" <naszar@ya.ru>
Date: Tue, 6 Jan 2015 19:19:44 -0800
Subject: [PATCH] Fix incorrect mount table entry parsing in __getmntent_r

When mount entry contains only four fields and have more then one space or
tab at the and, mp.mnt_freq and mp.mnt_passno will be set to some specific
values as side effect from parsing of previus mount entry. It is because
sscanf(""," %d %d ", &a, &b) returns -1, but this case is unprocessed.
Values of mp.mnt_freq and  mp.mnt_passno stays unchanged. This patch is
attempt to fix described issue by removing trailing tabs and spaces.
---
 ChangeLog         |  8 ++++++++
 NEWS              | 14 +++++++-------
 misc/mntent_r.c   |  6 +++++-
 misc/tst-mntent.c | 22 +++++++++++++++++++++-
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b5aa6e5..9ca4f27 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2015-01-06  Vladimir A. Nazarenko  <naszar@ya.ru>
+
+.	[BZ #17273]
+	* misc/mntent_r.c (__getmntent_r): Cut off trailing spaces
+	and tabs from buffer before parsing fstab entry.
+	* misc/tst-mntent.c (main): Add test for mount entry with
+	trailing spaces and tabs.
+
 2015-01-06  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #17748]
diff --git a/NEWS b/NEWS
index acb611a..8582885 100644
--- a/NEWS
+++ b/NEWS
@@ -11,13 +11,13 @@ Version 2.21
 
   6652, 10672, 12847, 12926, 13862, 14132, 14138, 14171, 14498, 15215,
   15884, 16191, 16469, 16617, 16619, 16657, 16740, 16857, 17192, 17266,
-  17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506,
-  17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17581, 17582,
-  17583, 17584, 17585, 17589, 17594, 17601, 17608, 17616, 17625, 17630,
-  17633, 17634, 17635, 17647, 17653, 17657, 17664, 17665, 17668, 17682,
-  17717, 17719, 17722, 17723, 17724, 17725, 17732, 17733, 17744, 17745,
-  17746, 17747, 17775, 17777, 17780, 17781, 17782, 17793, 17796, 17797,
-  17806
+  17273, 17344, 17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501,
+  17506, 17508, 17522, 17555, 17570, 17571, 17572, 17573, 17574, 17581,
+  17582, 17583, 17584, 17585, 17589, 17594, 17601, 17608, 17616, 17625,
+  17630, 17633, 17634, 17635, 17647, 17653, 17657, 17664, 17665, 17668,
+  17682, 17717, 17719, 17722, 17723, 17724, 17725, 17732, 17733, 17744,
+  17745, 17746, 17747, 17775, 17777, 17780, 17781, 17782, 17793, 17796,
+  17797, 17806
 
 * i386 memcpy functions optimized with SSE2 unaligned load/store.
 
diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index 152a9a2..6159873 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -135,7 +135,11 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 
       end_ptr = strchr (buffer, '\n');
       if (end_ptr != NULL)	/* chop newline */
-	*end_ptr = '\0';
+	{
+	  while (end_ptr[-1] == ' ' || end_ptr[-1] == '\t')
+            end_ptr--;
+	  *end_ptr = '\0';
+	}
       else
 	{
 	  /* Not the whole line was read.  Do it now but forget it.  */
diff --git a/misc/tst-mntent.c b/misc/tst-mntent.c
index 802b56e..876c89f 100644
--- a/misc/tst-mntent.c
+++ b/misc/tst-mntent.c
@@ -73,7 +73,27 @@ main (int argc, char *argv[])
 	  puts ("Error while reading written entry back in");
 	  result = 1;
 	}
-    }
+
+      /* Part III: Entry with whitespaces at the end of a line. */
+      rewind (fp);
+
+      fputs ("/foo\\040dir /bar\\040dir auto bind \t \n", fp);
+
+      rewind (fp);
+
+      mnt = getmntent (fp);
+
+      if (strcmp (mnt->mnt_fsname, "/foo dir") != 0
+	  || strcmp (mnt->mnt_dir, "/bar dir") != 0
+	  || strcmp (mnt->mnt_type, "auto") != 0
+	  || strcmp (mnt->mnt_opts, "bind") != 0
+	  || mnt->mnt_freq != 0
+	  || mnt->mnt_passno != 0)
+	{
+	  puts ("Error while reading entry with trailing whitespaces");
+	  result = 1;
+	}
+   }
 
   return result;
 }
-- 
1.9.3

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Joseph Myers]

On Wed, 7 Jan 2015, Vladimir A. Nazarenko wrote:

> On 07.01.2015 03:11, H.J. Lu wrote:
> > 
> > Has the copyright assignment issue been resolved?
> > 
> 
> Yes, I think so. I received signed paper from FSF. Should I send it to 
> someone?

I can confirm there is a copyright.list entry for Vladimir A. Nazarenko 
dated 2014-11-24, so no further action is needed there.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Vladimir A. Nazarenko]

On 07.01.2015 03:11, H.J. Lu wrote:
> 
> Has the copyright assignment issue been resolved?
> 

Yes, I think so. I received signed paper from FSF. Should I send it to someone?

Re: [PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[H.J. Lu]

On Tue, Dec 9, 2014 at 4:41 AM, Vladimir A. Nazarenko <naszar@ya.ru> wrote:
> When mount entry contains only four fields and have more
> then one space or tab at the and, mp.mnt_freq and
> mp.mnt_passno will be set to some specific values as side
> effect from parsing of previus mount entry. It is because
> sscanf(""," %d %d ", &a, &b) returns -1, but this case
> is  unprocessed. Values of mp.mnt_freq and  mp.mnt_passno
> stays unchanged. This patch is attempt to fix described issue
> by removing trailing tabs and spaces.
>
>         [BZ #17273]
>         * misc/mntent_r.c (__getmntent_r): Cut off trailing spaces
>         and tabs from buffer before parsing fstab entry.
>         * misc/tst-mntent.c (main): Add test for mount entry with
>         trailing spaces and tabs.
> ---
>  misc/mntent_r.c   |  6 +++++-
>  misc/tst-mntent.c | 22 +++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/misc/mntent_r.c b/misc/mntent_r.c
> index e68ec8e..e0a0b9d 100644
> --- a/misc/mntent_r.c
> +++ b/misc/mntent_r.c
> @@ -135,7 +135,11 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
>
>        end_ptr = strchr (buffer, '\n');
>        if (end_ptr != NULL)     /* chop newline */
> -       *end_ptr = '\0';
> +       {
> +         while (end_ptr[-1] == ' ' || end_ptr[-1] == '\t')
> +            end_ptr--;
> +         *end_ptr = '\0';
> +       }
>        else
>         {
>           /* Not the whole line was read.  Do it now but forget it.  */
> diff --git a/misc/tst-mntent.c b/misc/tst-mntent.c
> index 802b56e..876c89f 100644
> --- a/misc/tst-mntent.c
> +++ b/misc/tst-mntent.c
> @@ -73,7 +73,27 @@ main (int argc, char *argv[])
>           puts ("Error while reading written entry back in");
>           result = 1;
>         }
> -    }
> +
> +      /* Part III: Entry with whitespaces at the end of a line. */
> +      rewind (fp);
> +
> +      fputs ("/foo\\040dir /bar\\040dir auto bind \t \n", fp);
> +
> +      rewind (fp);
> +
> +      mnt = getmntent (fp);
> +
> +      if (strcmp (mnt->mnt_fsname, "/foo dir") != 0
> +         || strcmp (mnt->mnt_dir, "/bar dir") != 0
> +         || strcmp (mnt->mnt_type, "auto") != 0
> +         || strcmp (mnt->mnt_opts, "bind") != 0
> +         || mnt->mnt_freq != 0
> +         || mnt->mnt_passno != 0)
> +       {
> +         puts ("Error while reading entry with trailing whitespaces");
> +         result = 1;
> +       }
> +   }
>
>    return result;
>  }
> --
> 2.1.1
>

Has the copyright assignment issue been resolved?

-- 
H.J.

[PATCH v3] [BZ #17273] fix incorrect mount table entry parsing in __getmntent_r()

[Vladimir A. Nazarenko]

When mount entry contains only four fields and have more
then one space or tab at the and, mp.mnt_freq and
mp.mnt_passno will be set to some specific values as side
effect from parsing of previus mount entry. It is because
sscanf(""," %d %d ", &a, &b) returns -1, but this case
is  unprocessed. Values of mp.mnt_freq and  mp.mnt_passno
stays unchanged. This patch is attempt to fix described issue
by removing trailing tabs and spaces.

	[BZ #17273]
	* misc/mntent_r.c (__getmntent_r): Cut off trailing spaces
	and tabs from buffer before parsing fstab entry.
	* misc/tst-mntent.c (main): Add test for mount entry with
	trailing spaces and tabs.
---
 misc/mntent_r.c   |  6 +++++-
 misc/tst-mntent.c | 22 +++++++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/misc/mntent_r.c b/misc/mntent_r.c
index e68ec8e..e0a0b9d 100644
--- a/misc/mntent_r.c
+++ b/misc/mntent_r.c
@@ -135,7 +135,11 @@ __getmntent_r (FILE *stream, struct mntent *mp, char *buffer, int bufsiz)
 
       end_ptr = strchr (buffer, '\n');
       if (end_ptr != NULL)	/* chop newline */
-	*end_ptr = '\0';
+	{
+	  while (end_ptr[-1] == ' ' || end_ptr[-1] == '\t')
+            end_ptr--;
+	  *end_ptr = '\0';
+	}
       else
 	{
 	  /* Not the whole line was read.  Do it now but forget it.  */
diff --git a/misc/tst-mntent.c b/misc/tst-mntent.c
index 802b56e..876c89f 100644
--- a/misc/tst-mntent.c
+++ b/misc/tst-mntent.c
@@ -73,7 +73,27 @@ main (int argc, char *argv[])
 	  puts ("Error while reading written entry back in");
 	  result = 1;
 	}
-    }
+
+      /* Part III: Entry with whitespaces at the end of a line. */
+      rewind (fp);
+
+      fputs ("/foo\\040dir /bar\\040dir auto bind \t \n", fp);
+
+      rewind (fp);
+
+      mnt = getmntent (fp);
+
+      if (strcmp (mnt->mnt_fsname, "/foo dir") != 0
+	  || strcmp (mnt->mnt_dir, "/bar dir") != 0
+	  || strcmp (mnt->mnt_type, "auto") != 0
+	  || strcmp (mnt->mnt_opts, "bind") != 0
+	  || mnt->mnt_freq != 0
+	  || mnt->mnt_passno != 0)
+	{
+	  puts ("Error while reading entry with trailing whitespaces");
+	  result = 1;
+	}
+   }
 
   return result;
 }
-- 
2.1.1