| Message ID | CALoOobPh5bGmXKTiGua=RoSO=ow=jaHpEseECJX8_GJ+Rde85g@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 03/05/2015 11:33 PM, Paul Pluzhnikov wrote: > On Thu, Mar 5, 2015 at 11:48 AM, Carlos O'Donell <carlos@redhat.com> wrote: > >> > OK to commit as long as you verified that test case fails before >> > and passes afterwards on at least x86_64. > It doesn't. To make it fail I would have to mmap two pages, mprotect > the second, and place the string at the end of the first page... which > is quite a bit of code. > > Must I do that? I guess if we hope to catch any regression here, I must... Yes, we must. It's painful, but it's one of the only ways we accelerate the development pace of the project and keep the quality high. We have to trust that our testing is getting better and covering more of the cases we catch. Eventually maybe we'll generalize your test and run all of the cases like this with the string ending on a page boundary. > Attached patch is verified to fail posix/wordexp-test with expected > SIGSEGV due to overflow before the fix, and pass after. Excellent. > 2015-03-05 Paul Pluzhnikov <ppluzhnikov@google.com> > > [BZ #18043] > * posix/wordexp.c (parse_param): Fix buffer overflow. > * posix/wordexp-test.c (do_bz18043): Add test case. Perfect. Please check it in. > -- Paul Pluzhnikov > > > bz18043.patch2.txt > > > diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c > index 8a312e0..c928438 100644 > --- a/posix/wordexp-test.c > +++ b/posix/wordexp-test.c > @@ -17,6 +17,7 @@ > > #include <sys/stat.h> > #include <sys/types.h> > +#include <sys/mman.h> > #include <fcntl.h> > #include <unistd.h> > #include <pwd.h> > @@ -249,6 +250,31 @@ command_line_test (const char *words) > printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); > } > > +static int > +do_bz18043 (void) > +{ > + const int pagesize = getpagesize (); > + char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, > + MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); > + > + if (start == MAP_FAILED) > + return 1; > + > + if (mprotect (start + pagesize, pagesize, PROT_NONE)) > + return 2; > + > + const char word[] = "${"; > + char *word_start = start + pagesize - sizeof (word); > + memcpy (word_start, word, sizeof (word)); > + > + wordexp_t w; > + wordexp (word_start, &w, 0); > + > + munmap (start, 2 * pagesize); > + > + return 0; > +} > + > int > main (int argc, char *argv[]) > { > @@ -370,6 +396,9 @@ main (int argc, char *argv[]) > > printf ("tests failed: %d\n", fail); > > + if (do_bz18043 ()) > + ++fail; > + > return fail != 0; > } > > diff --git a/posix/wordexp.c b/posix/wordexp.c > index e3d8d6b..1c14401 100644 > --- a/posix/wordexp.c > +++ b/posix/wordexp.c > @@ -1299,7 +1299,7 @@ parse_param (char **word, size_t *word_length, size_t *max_length, > } > while (isdigit(words[++*offset])); > } > - else if (strchr ("*@$", words[*offset]) != NULL) > + else if (words[*offset] != '\0' && strchr ("*@$", words[*offset]) != NULL) > { > /* Special parameter. */ > special = 1;
On Fri, Mar 6, 2015 at 7:21 AM, Carlos O'Donell <carlos@redhat.com> wrote: >> + wordexp_t w; >> + wordexp (word_start, &w, 0); This lost the check for expected failure in wordexp from the original patch. I've added it back in and committed as 895c30cb003857b52c1675f9078e6a799b231bcb. I'll generalize the check for all other patterns, then update patch for BZ 18042. Thanks,
diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c index 8a312e0..c928438 100644 --- a/posix/wordexp-test.c +++ b/posix/wordexp-test.c @@ -17,6 +17,7 @@ #include <sys/stat.h> #include <sys/types.h> +#include <sys/mman.h> #include <fcntl.h> #include <unistd.h> #include <pwd.h> @@ -249,6 +250,31 @@ command_line_test (const char *words) printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]); } +static int +do_bz18043 (void) +{ + const int pagesize = getpagesize (); + char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE, + MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); + + if (start == MAP_FAILED) + return 1; + + if (mprotect (start + pagesize, pagesize, PROT_NONE)) + return 2; + + const char word[] = "${"; + char *word_start = start + pagesize - sizeof (word); + memcpy (word_start, word, sizeof (word)); + + wordexp_t w; + wordexp (word_start, &w, 0); + + munmap (start, 2 * pagesize); + + return 0; +} + int main (int argc, char *argv[]) { @@ -370,6 +396,9 @@ main (int argc, char *argv[]) printf ("tests failed: %d\n", fail); + if (do_bz18043 ()) + ++fail; + return fail != 0; } diff --git a/posix/wordexp.c b/posix/wordexp.c index e3d8d6b..1c14401 100644 --- a/posix/wordexp.c +++ b/posix/wordexp.c @@ -1299,7 +1299,7 @@ parse_param (char **word, size_t *word_length, size_t *max_length, } while (isdigit(words[++*offset])); } - else if (strchr ("*@$", words[*offset]) != NULL) + else if (words[*offset] != '\0' && strchr ("*@$", words[*offset]) != NULL) { /* Special parameter. */ special = 1;