Merge into trunk : fix-for-bug-834813 : Code : libnih

Status:

Merged

Merged at revision:

1053

Proposed branch:

lp://staging/~jamesodhunt/libnih/fix-for-bug-834813

Merge into:

lp://staging/libnih

Diff against target:

440 lines (+396/-1)

3 files modified

ChangeLog (+8/-0)
nih/string.c (+8/-1)
nih/tests/test_string.c (+380/-0)

To merge this branch:

bzr merge lp://staging/~jamesodhunt/libnih/fix-for-bug-834813

Undecided

Fix Committed

Link a bug report

Reviewer	Review Type	Date Requested	Status
Scott James Remnant		2011-08-31	Pending
Review via email: mp+73575@code.staging.launchpad.net

This proposal supersedes a proposal from 2011-08-31.

Description of the change

Final attempt I hope.

  * ChangeLog: updated.
  * nih/string.c (nih_str_split): Fixes to avoid over-running
    input string and also returning an empty string array entry
    when repeat is true (LP: #834813).
  * nih/tests/test_string.c (test_str_split): Added a lot of new
    tests for nih_str_split().

Revision history for this message

Scott James Remnant (scott) wrote on 2011-08-26: Posted in a previous version of this proposal

#

"str && *str" shouldn't be necessary, this function already asserts "str != NULL"

Have you checked other uses of strchr() in the code, I probably make the same mistake all over the place - believing it would return NULL if given '\0'

I'd like that final if() cleaned up, it doesn't make sense what you're doing there - perhaps splitting out some informationally named temporary variables and checking those would make more sense, or using some other method

review: Needs Fixing

Revision history for this message

James Hunt (jamesodhunt) wrote on 2011-08-30: Posted in a previous version of this proposal

#

> "str && *str" shouldn't be necessary, this function already asserts "str !=
> NULL"
>
> Have you checked other uses of strchr() in the code, I probably make the same
> mistake all over the place - believing it would return NULL if given '\0'
>
> I'd like that final if() cleaned up, it doesn't make sense what you're doing
> there - perhaps splitting out some informationally named temporary variables
> and checking those would make more sense, or using some other method
Hi Scott,

Firstly, I've raised a bug on strchr(3) and memchr(3) since they do not currently specify the behaviour should the char to search for be '\0':

https://bugzilla.kernel.org/show_bug.cgi?id=42042

I've reworked (simplified) the change to nih_str_split(). Note that the "continue" is safe since we consume any and all multiple delimiter characters on the next loop iteration.

The assert protects the function initially, but since we're incrementing 'str' we do need atleast the first "str && *str" check...

while (repeat && str && *str && strchr (delim, *str))

...since without it, we overrun the string, and the new tests will fail. The subsequent "str && *str"'s may not be strictly required, but as this bug was rather subtle anyway, I'd be tempted to err on the side of caution.

I'll resubmit the MP...

Cheers,

James.

Revision history for this message

Scott James Remnant (scott) wrote on 2011-08-30: Posted in a previous version of this proposal

#

Download full text (18.3 KiB)

str can never turn to NULL as a result of incrementing

On Tue, Aug 30, 2011 at 7:33 AM, James Hunt <email address hidden>wrote:

> James Hunt has proposed merging lp:~jamesodhunt/libnih/fix-for-bug-834813
> into lp:libnih.
>
> Requested reviews:
> Scott James Remnant (scott)
>
> For more details, see:
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73383
>
> * ChangeLog: updated.
> * nih/string.c (nih_str_split): Fixes to avoid over-running
> input string and also returning an empty string array entry
> when repeat is true (LP: #834813).
> * nih/tests/test_string.c (test_str_split): Added a lot of new
> tests for nih_str_split().
>
> --
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73383
> You are requested to review the proposed merge of
> lp:~jamesodhunt/libnih/fix-for-bug-834813 into lp:libnih.
>
> === modified file 'ChangeLog'
> --- ChangeLog 2011-08-26 18:31:43 +0000
> +++ ChangeLog 2011-08-30 14:33:16 +0000
> @@ -1,3 +1,4 @@
> +<<<<<<< TREE
> 2011-08-26 James Hunt <email address hidden>
>
> * nih/io.c (nih_io_select_fds): Ensure number of fds being managed
> @@ -6,6 +7,16 @@
> * nih/config.c, nih/error.h, nih/io.c, nih/test_files.h: Correct
> typos in comments.
>
> +=======
> +2011-08-30 James Hunt <email address hidden>
> +
> + * nih/string.c (nih_str_split): Fixes to avoid over-running
> + input string and also returning an empty string array entry
> + when repeat is true (LP: #834813).
> + * nih/tests/test_string.c (test_str_split): Added a lot of new
> + tests for nih_str_split().
> +
> +>>>>>>> MERGE-SOURCE
> 2011-06-20 James Hunt <email address hidden>
>
> * nih/watch.c (nih_watch_handle): Handle non-directory watches;
>
> === modified file 'nih/string.c'
> --- nih/string.c 2009-06-23 09:29:37 +0000
> +++ nih/string.c 2011-08-30 14:33:16 +0000
> @@ -403,17 +403,29 @@
>
> while (*str) {
> const char *ptr;
> + size_t toklen;
>
> - /* Skip initial delimiters */
> - while (repeat && strchr (delim, *str))
> + /* Skip initial delimiters taking care not to walk
> + * beyond the end of the string
> + */
> + while (repeat && str && *str && strchr (delim, *str))
> str++;
>
> /* Find the end of the token */
> ptr = str;
> - while (*str && (! strchr (delim, *str)))
> + while (str && *str && (! strchr (delim, *str)))
> str++;
>
> - if (! nih_str_array_addn (&array, parent, &len,
> + toklen = str - ptr;
> +
> + /* Don't create an empty string array element in repeat
> + * mode if there is no token (as a result of a
> + * duplicated delimiter character).
> + */
> + if (repeat && !toklen)
> + continue;
> +
> + if (str && ! nih_str_array_addn (&array, parent, &len,
> ptr, str ...

str can never turn to NULL as a result of incrementing

On Tue, Aug 30, 2011 at 7:33 AM, James Hunt <james.hunt@canonical.com>wrote:

> James Hunt has proposed merging lp:~jamesodhunt/libnih/fix-for-bug-834813
> into lp:libnih.
>
> Requested reviews:
>  Scott James Remnant (scott)
>
> For more details, see:
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73383
>
>  * ChangeLog: updated.
>  * nih/string.c (nih_str_split): Fixes to avoid over-running
>    input string and also returning an empty string array entry
>    when repeat is true (LP: #834813).
>  * nih/tests/test_string.c (test_str_split): Added a lot of new
>    tests for nih_str_split().
>
> --
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73383
> You are requested to review the proposed merge of
> lp:~jamesodhunt/libnih/fix-for-bug-834813 into lp:libnih.
>
> === modified file 'ChangeLog'
> --- ChangeLog   2011-08-26 18:31:43 +0000
> +++ ChangeLog   2011-08-30 14:33:16 +0000
> @@ -1,3 +1,4 @@
> +<<<<<<< TREE
>  2011-08-26  James Hunt  <james.hunt@ubuntu.com>
>
>        * nih/io.c (nih_io_select_fds): Ensure number of fds being managed
> @@ -6,6 +7,16 @@
>        * nih/config.c, nih/error.h, nih/io.c, nih/test_files.h: Correct
>        typos in comments.
>
> +=======
> +2011-08-30  James Hunt  <james.hunt@ubuntu.com>
> +
> +       * nih/string.c (nih_str_split): Fixes to avoid over-running
> +         input string and also returning an empty string array entry
> +         when repeat is true (LP: #834813).
> +       * nih/tests/test_string.c (test_str_split): Added a lot of new
> +         tests for nih_str_split().
> +
> +>>>>>>> MERGE-SOURCE
>  2011-06-20  James Hunt  <james.hunt@ubuntu.com>
>
>        * nih/watch.c (nih_watch_handle): Handle non-directory watches;
>
> === modified file 'nih/string.c'
> --- nih/string.c        2009-06-23 09:29:37 +0000
> +++ nih/string.c        2011-08-30 14:33:16 +0000
> @@ -403,17 +403,29 @@
>
>        while (*str) {
>                const char  *ptr;
> +               size_t       toklen;
>
> -               /* Skip initial delimiters */
> -               while (repeat && strchr (delim, *str))
> +               /* Skip initial delimiters taking care not to walk
> +                * beyond the end of the string
> +                */
> +               while (repeat && str && *str && strchr (delim, *str))
>                        str++;
>
>                /* Find the end of the token */
>                ptr = str;
> -               while (*str && (! strchr (delim, *str)))
> +               while (str && *str && (! strchr (delim, *str)))
>                        str++;
>
> -               if (! nih_str_array_addn (&array, parent, &len,
> +               toklen = str - ptr;
> +
> +               /* Don't create an empty string array element in repeat
> +                * mode if there is no token (as a result of a
> +                * duplicated delimiter character).
> +                */
> +               if (repeat && !toklen)
> +                       continue;
> +
> +               if (str && ! nih_str_array_addn (&array, parent, &len,
>                                          ptr, str - ptr)) {
>                        nih_free (array);
>                        return NULL;
>
> === modified file 'nih/tests/test_string.c'
> --- nih/tests/test_string.c     2009-11-28 20:25:03 +0000
> +++ nih/tests/test_string.c     2011-08-30 14:33:16 +0000
> @@ -619,6 +619,215 @@
>                nih_free (array);
>        }
>
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "\t\tthis is a test", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 7);
> +               for (i = 0; i < 6; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "this");
> +               TEST_EQ_STR (array[3], "is");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "test");
> +               TEST_EQ_P (array[6], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, " \tthis is a test", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 7);
> +               for (i = 0; i < 6; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "this");
> +               TEST_EQ_STR (array[3], "is");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "test");
> +               TEST_EQ_P (array[6], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is   a\t\ttest", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 8);
> +               for (i = 0; i < 7; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "");
> +               TEST_EQ_STR (array[6], "test");
> +               TEST_EQ_P (array[7], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is \n\ta\ttest", "
> \t\n", FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 7);
> +               for (i = 0; i < 6; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "test");
> +               TEST_EQ_P (array[6], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is a test  ", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 6);
> +               for (i = 0; i < 5; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "a");
> +               TEST_EQ_STR (array[3], "test");
> +               TEST_EQ_STR (array[4], "");
> +               TEST_EQ_P (array[5], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is a test \t", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 6);
> +               for (i = 0; i < 5; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "a");
> +               TEST_EQ_STR (array[3], "test");
> +               TEST_EQ_STR (array[4], "");
> +               TEST_EQ_P (array[5], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "   this is\n\n\na
> test\t\t\t", " \t\n", FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 12);
> +               for (i = 0; i < 11; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "this");
> +               TEST_EQ_STR (array[4], "is");
> +               TEST_EQ_STR (array[5], "");
> +               TEST_EQ_STR (array[6], "");
> +               TEST_EQ_STR (array[7], "a");
> +               TEST_EQ_STR (array[8], "test");
> +               TEST_EQ_STR (array[9], "");
> +               TEST_EQ_STR (array[10], "");
> +               TEST_EQ_P (array[11], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, ": \nthis is\t \n:a test:\n ",
> "\n :\t", FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 13);
> +               for (i = 0; i < 12; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "this");
> +               TEST_EQ_STR (array[4], "is");
> +               TEST_EQ_STR (array[5], "");
> +               TEST_EQ_STR (array[6], "");
> +               TEST_EQ_STR (array[7], "");
> +               TEST_EQ_STR (array[8], "a");
> +               TEST_EQ_STR (array[9], "test");
> +               TEST_EQ_STR (array[10], "");
> +               TEST_EQ_STR (array[11], "");
> +               TEST_EQ_P (array[12], NULL);
> +
> +               nih_free (array);
> +       }
>
>        /* Check that we can split a string treating multiple consecutive
>         * matching characters as a single separator to be skipped.
> @@ -645,6 +854,177 @@
>                nih_free (array);
>        }
>
> +       /* Check that we can split a string containing multiple
> +        * occurences of one of the delimiter characters at the
> +        * beginning of the string.
> +        */
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "\n\nhello", " \t\r\n", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "\n\r hello", " \t\r\n",
> TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello\n\rworld", " \t\n\r",
> TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 3);
> +               for (i = 0; i < 2; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], "world");
> +               TEST_EQ_P (array[2], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello\n\r\tworld", " \t\n\r",
> TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 3);
> +               for (i = 0; i < 2; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], "world");
> +               TEST_EQ_P (array[2], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello\n\n\n\n\n\n\n", "
> \t\r\n", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello \r\t\r\t\n ", "
> \t\r\n", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL,
> +                               "        hello\n\n\n,  world\n\n\n",
> +                               "\r\t\n ", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 4);
> +               for (i = 0; i < 3; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], ",");
> +               TEST_EQ_STR (array[2], "world");
> +               TEST_EQ_P (array[3], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL,
> +                               "\n    \r\thello\n\n\r , \n\t\rworld\t \r\n
> \n",
> +                               " \t\n\r", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 4);
> +               for (i = 0; i < 3; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], ",");
> +               TEST_EQ_STR (array[2], "world");
> +               TEST_EQ_P (array[3], NULL);
> +
> +               nih_free (array);
> +       }
>
>        /* Check that we can give an empty string, and end up with a
>         * one-element array that only contains a NULL pointer.
>
>
>

Revision history for this message

Scott James Remnant (scott) wrote on 2011-08-31: Posted in a previous version of this proposal

#

Download full text (19.0 KiB)

Last comments are all nits (I promise!)

ChangeLog has merge markers in the review diff;

and today is now the 31st, so this needs a ChangeLog header of its own.

- /* Skip initial delimiters */
- while (repeat && strchr (delim, *str))
+ /* Skip initial delimiters taking care not to walk
+ * beyond the end of the string
+ */

There's no need to change this comment, leave it as it is.

+ toklen = str - ptr;
+
+ /* Don't create an empty string array element in repeat
+ * mode if there is no token (as a result of a
+ * duplicated delimiter character).
+ */
+ if (repeat && !toklen)
+ continue;
+

Calculating the length of the token and checking it is completely
unnecessary. A much better check would be:

if (repeat && (str == ptr))

This is much clearer that you mean "if we're repeating, and the end of the
string is the start"

On Wed, Aug 31, 2011 at 10:02 AM, James Hunt <email address hidden>wrote:

> James Hunt has proposed merging lp:~jamesodhunt/libnih/fix-for-bug-834813
> into lp:libnih.
>
> Requested reviews:
> Scott James Remnant (scott)
>
> For more details, see:
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73560
>
> Take 3... :-)
>
> * ChangeLog: updated.
> * nih/string.c (nih_str_split): Fixes to avoid over-running
> input string and also returning an empty string array entry
> when repeat is true (LP: #834813).
> * nih/tests/test_string.c (test_str_split): Added a lot of new
> tests for nih_str_split().
>
> --
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73560
> You are requested to review the proposed merge of
> lp:~jamesodhunt/libnih/fix-for-bug-834813 into lp:libnih.
>
> === modified file 'ChangeLog'
> --- ChangeLog 2011-08-26 18:31:43 +0000
> +++ ChangeLog 2011-08-31 17:02:23 +0000
> @@ -1,3 +1,4 @@
> +<<<<<<< TREE
> 2011-08-26 James Hunt <email address hidden>
>
> * nih/io.c (nih_io_select_fds): Ensure number of fds being managed
> @@ -6,6 +7,16 @@
> * nih/config.c, nih/error.h, nih/io.c, nih/test_files.h: Correct
> typos in comments.
>
> +=======
> +2011-08-31 James Hunt <email address hidden>
> +
> + * nih/string.c (nih_str_split): Fixes to avoid over-running
> + input string and also returning an empty string array entry
> + when repeat is true (LP: #834813).
> + * nih/tests/test_string.c (test_str_split): Added a lot of new
> + tests for nih_str_split().
> +
> +>>>>>>> MERGE-SOURCE
> 2011-06-20 James Hunt <email address hidden>
>
> * nih/watch.c (nih_watch_handle): Handle non-directory watches;
>
> === modified file 'nih/string.c'
> --- nih/string.c 2009-06-23 09:29:37 +0000
> +++ nih/string.c 2011-08-31 17:02:23 +0000
> @@ -403,9 +403,12 @@
>
> while (*str) {
> const char *ptr;
> + size_t toklen;
>
> - /* Skip initial delimiters */
> - while (repeat && strchr (delim, *str))
> + /* Skip initi...

Last comments are all nits (I promise!)

ChangeLog has merge markers in the review diff;

and today is now the 31st, so this needs a ChangeLog header of its own.

-               /* Skip initial delimiters */
-               while (repeat && strchr (delim, *str))
+               /* Skip initial delimiters taking care not to walk
+                * beyond the end of the string
+                */

There's no need to change this comment, leave it as it is.

+               toklen = str - ptr;
+
+               /* Don't create an empty string array element in repeat
+                * mode if there is no token (as a result of a
+                * duplicated delimiter character).
+                */
+               if (repeat && !toklen)
+                       continue;
+

Calculating the length of the token and checking it is completely
unnecessary. A much better check would be:

if (repeat && (str == ptr))

This is much clearer that you mean "if we're repeating, and the end of the
string is the start"

On Wed, Aug 31, 2011 at 10:02 AM, James Hunt <james.hunt@canonical.com>wrote:

> James Hunt has proposed merging lp:~jamesodhunt/libnih/fix-for-bug-834813
> into lp:libnih.
>
> Requested reviews:
>  Scott James Remnant (scott)
>
> For more details, see:
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73560
>
> Take 3... :-)
>
>  * ChangeLog: updated.
>  * nih/string.c (nih_str_split): Fixes to avoid over-running
>    input string and also returning an empty string array entry
>    when repeat is true (LP: #834813).
>  * nih/tests/test_string.c (test_str_split): Added a lot of new
>    tests for nih_str_split().
>
> --
>
> https://code.launchpad.net/~jamesodhunt/libnih/fix-for-bug-834813/+merge/73560
> You are requested to review the proposed merge of
> lp:~jamesodhunt/libnih/fix-for-bug-834813 into lp:libnih.
>
> === modified file 'ChangeLog'
> --- ChangeLog   2011-08-26 18:31:43 +0000
> +++ ChangeLog   2011-08-31 17:02:23 +0000
> @@ -1,3 +1,4 @@
> +<<<<<<< TREE
>  2011-08-26  James Hunt  <james.hunt@ubuntu.com>
>
>        * nih/io.c (nih_io_select_fds): Ensure number of fds being managed
> @@ -6,6 +7,16 @@
>        * nih/config.c, nih/error.h, nih/io.c, nih/test_files.h: Correct
>        typos in comments.
>
> +=======
> +2011-08-31  James Hunt  <james.hunt@ubuntu.com>
> +
> +       * nih/string.c (nih_str_split): Fixes to avoid over-running
> +         input string and also returning an empty string array entry
> +         when repeat is true (LP: #834813).
> +       * nih/tests/test_string.c (test_str_split): Added a lot of new
> +         tests for nih_str_split().
> +
> +>>>>>>> MERGE-SOURCE
>  2011-06-20  James Hunt  <james.hunt@ubuntu.com>
>
>        * nih/watch.c (nih_watch_handle): Handle non-directory watches;
>
> === modified file 'nih/string.c'
> --- nih/string.c        2009-06-23 09:29:37 +0000
> +++ nih/string.c        2011-08-31 17:02:23 +0000
> @@ -403,9 +403,12 @@
>
>        while (*str) {
>                const char  *ptr;
> +               size_t       toklen;
>
> -               /* Skip initial delimiters */
> -               while (repeat && strchr (delim, *str))
> +               /* Skip initial delimiters taking care not to walk
> +                * beyond the end of the string
> +                */
> +               while (repeat && *str && strchr (delim, *str))
>                        str++;
>
>                /* Find the end of the token */
> @@ -413,6 +416,15 @@
>                while (*str && (! strchr (delim, *str)))
>                        str++;
>
> +               toklen = str - ptr;
> +
> +               /* Don't create an empty string array element in repeat
> +                * mode if there is no token (as a result of a
> +                * duplicated delimiter character).
> +                */
> +               if (repeat && !toklen)
> +                       continue;
> +
>                if (! nih_str_array_addn (&array, parent, &len,
>                                          ptr, str - ptr)) {
>                        nih_free (array);
>
> === modified file 'nih/tests/test_string.c'
> --- nih/tests/test_string.c     2009-11-28 20:25:03 +0000
> +++ nih/tests/test_string.c     2011-08-31 17:02:23 +0000
> @@ -619,6 +619,215 @@
>                nih_free (array);
>        }
>
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "\t\tthis is a test", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 7);
> +               for (i = 0; i < 6; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "this");
> +               TEST_EQ_STR (array[3], "is");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "test");
> +               TEST_EQ_P (array[6], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, " \tthis is a test", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 7);
> +               for (i = 0; i < 6; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "this");
> +               TEST_EQ_STR (array[3], "is");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "test");
> +               TEST_EQ_P (array[6], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is   a\t\ttest", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 8);
> +               for (i = 0; i < 7; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "");
> +               TEST_EQ_STR (array[6], "test");
> +               TEST_EQ_P (array[7], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is \n\ta\ttest", "
> \t\n", FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 7);
> +               for (i = 0; i < 6; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "");
> +               TEST_EQ_STR (array[4], "a");
> +               TEST_EQ_STR (array[5], "test");
> +               TEST_EQ_P (array[6], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is a test  ", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 6);
> +               for (i = 0; i < 5; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "a");
> +               TEST_EQ_STR (array[3], "test");
> +               TEST_EQ_STR (array[4], "");
> +               TEST_EQ_P (array[5], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "this is a test \t", " \t",
> FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 6);
> +               for (i = 0; i < 5; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "this");
> +               TEST_EQ_STR (array[1], "is");
> +               TEST_EQ_STR (array[2], "a");
> +               TEST_EQ_STR (array[3], "test");
> +               TEST_EQ_STR (array[4], "");
> +               TEST_EQ_P (array[5], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple identical delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "   this is\n\n\na
> test\t\t\t", " \t\n", FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 12);
> +               for (i = 0; i < 11; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "this");
> +               TEST_EQ_STR (array[4], "is");
> +               TEST_EQ_STR (array[5], "");
> +               TEST_EQ_STR (array[6], "");
> +               TEST_EQ_STR (array[7], "a");
> +               TEST_EQ_STR (array[8], "test");
> +               TEST_EQ_STR (array[9], "");
> +               TEST_EQ_STR (array[10], "");
> +               TEST_EQ_P (array[11], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with no repeat and multiple different delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, ": \nthis is\t \n:a test:\n ",
> "\n :\t", FALSE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 13);
> +               for (i = 0; i < 12; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "");
> +               TEST_EQ_STR (array[1], "");
> +               TEST_EQ_STR (array[2], "");
> +               TEST_EQ_STR (array[3], "this");
> +               TEST_EQ_STR (array[4], "is");
> +               TEST_EQ_STR (array[5], "");
> +               TEST_EQ_STR (array[6], "");
> +               TEST_EQ_STR (array[7], "");
> +               TEST_EQ_STR (array[8], "a");
> +               TEST_EQ_STR (array[9], "test");
> +               TEST_EQ_STR (array[10], "");
> +               TEST_EQ_STR (array[11], "");
> +               TEST_EQ_P (array[12], NULL);
> +
> +               nih_free (array);
> +       }
>
>        /* Check that we can split a string treating multiple consecutive
>         * matching characters as a single separator to be skipped.
> @@ -645,6 +854,177 @@
>                nih_free (array);
>        }
>
> +       /* Check that we can split a string containing multiple
> +        * occurences of one of the delimiter characters at the
> +        * beginning of the string.
> +        */
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "\n\nhello", " \t\r\n", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter characters at string start");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "\n\r hello", " \t\r\n",
> TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello\n\rworld", " \t\n\r",
> TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 3);
> +               for (i = 0; i < 2; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], "world");
> +               TEST_EQ_P (array[2], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter "
> +                       "characters within string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello\n\r\tworld", " \t\n\r",
> TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 3);
> +               for (i = 0; i < 2; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], "world");
> +               TEST_EQ_P (array[2], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello\n\n\n\n\n\n\n", "
> \t\r\n", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter "
> +                       "characters at string end");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL, "hello \r\t\r\t\n ", "
> \t\r\n", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 2);
> +               for (i = 0; i < 1; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_P (array[1], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple identical adjacent
> delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL,
> +                               "        hello\n\n\n,  world\n\n\n",
> +                               "\r\t\n ", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 4);
> +               for (i = 0; i < 3; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], ",");
> +               TEST_EQ_STR (array[2], "world");
> +               TEST_EQ_P (array[3], NULL);
> +
> +               nih_free (array);
> +       }
> +
> +       TEST_FEATURE ("with repeat and multiple different adjacent
> delimiter "
> +                       "characters at beginning, middle and end of
> string");
> +       TEST_ALLOC_FAIL {
> +               array = nih_str_split (NULL,
> +                               "\n    \r\thello\n\n\r , \n\t\rworld\t \r\n
> \n",
> +                               " \t\n\r", TRUE);
> +
> +               if (test_alloc_failed) {
> +                       TEST_EQ_P (array, NULL);
> +                       continue;
> +               }
> +
> +               TEST_ALLOC_SIZE (array, sizeof (char *) * 4);
> +               for (i = 0; i < 3; i++)
> +                       TEST_ALLOC_PARENT (array[i], array);
> +
> +               TEST_EQ_STR (array[0], "hello");
> +               TEST_EQ_STR (array[1], ",");
> +               TEST_EQ_STR (array[2], "world");
> +               TEST_EQ_P (array[3], NULL);
> +
> +               nih_free (array);
> +       }
>
>        /* Check that we can give an empty string, and end up with a
>         * one-element array that only contains a NULL pointer.
>
>
>

Revision history for this message

Scott James Remnant (scott) wrote on 2011-08-31: Posted in a previous version of this proposal

#

On Tue, Aug 30, 2011 at 7:33 AM, James Hunt <email address hidden>wrote:

> Firstly, I've raised a bug on strchr(3) and memchr(3) since they do not
> currently specify the behaviour should the char to search for be '\0':
>
> https://bugzilla.kernel.org/show_bug.cgi?id=42042
>
>
Interestingly the OS X man pages do specify:

     The strchr() function locates the first occurrence of c (converted to a
     char) in the string pointed to by s. The terminating null character is
     considered to be part of the string; therefore if c is `\0', the func-
     tions locate the terminating `\0'.

Scott

libnih

Merge lp://staging/~jamesodhunt/libnih/fix-for-bug-834813 into lp://staging/libnih

Commit message

Description of the change

Preview Diff

Subscribers