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

[James Hunt]

  * 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().

[Scott James Remnant (scott)]

"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

[lp:~jamesodhunt/libnih/fix-for-bug-834813]

1050. By Scott James Remnant (Canonical) on 2011-08-26

* nih/config.c, nih/error.h, nih/io.c, nih/test_files.h: Correct
typos in comments.

1051. By Scott James Remnant (Canonical) on 2011-08-26

Bump copyright dates to 2011

1052. By Scott James Remnant (Canonical) on 2011-08-26

* nih/io.c (nih_io_select_fds): Ensure number of fds being managed
is within limits.

[James Hunt (jamesodhunt)]

> "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.

[Scott James Remnant (scott)]

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

[lp:~jamesodhunt/libnih/fix-for-bug-834813]

1053. By James Hunt on 2011-08-31

* 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().

Unmerged revisions

=== modified file 'ChangeLog'
--- ChangeLog	2011-08-26 18:31:43 +0000
+++ ChangeLog	2011-08-30 14:13:47 +0000
@@ -1,3 +1,4 @@
+<<<<<<< TREE
 2011-08-26  James Hunt  <[e-mail]>
 
 	* 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.
 
+=======
[phone]  James Hunt  <[e-mail]>
+
+	* 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  <[e-mail]>
 
 	* 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:13:47 +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:13:47 +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.