Comment on attachment 25119 [details] [review]
Improved _dbus_keyring_validate_context() and introduction of _dbus_string_find_char_from_sorted_array()

Review of attachment 25119 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-keyring.c
@@ +883,5 @@
> +            _dbus_verbose ("context contains a dot\n");
> +            break;
> +          case '/':
> +            _dbus_verbose ("context contains a slash\n");
> +            break;

case '\\': is missing.

::: dbus/dbus-string-util.c
@@ +690,5 @@
> +    _dbus_assert_not_reached ("Did find 'abc'");
> +  _dbus_assert (i == -1);
> +
> +  if (!_dbus_string_find_char_from_sorted_array(&str, 0, _dbus_string_get_length (&str), "abcl", &i))
> +    _dbus_assert_not_reached ("Didn't find 'l'");

Should be "Didn't find 'abcl'".

@@ +694,5 @@
> +    _dbus_assert_not_reached ("Didn't find 'l'");
> +  _dbus_assert (i == 2);
> +
> +  if (!_dbus_string_find_char_from_sorted_array(&str, 0, _dbus_string_get_length (&str), "cel", &i))
> +    _dbus_assert_not_reached ("Didn't find 'e'");

ditto.

@@ +702,5 @@
> +    _dbus_assert_not_reached ("Did find 'abc'");
> +  _dbus_assert (i == -1);
> +
> +  if (!_dbus_string_find_char_from_sorted_array(&str, 2, _dbus_string_get_length (&str) - 1, "abcl", &i))
> +    _dbus_assert_not_reached ("Didn't find 'l'");

ditto.

@@ +706,5 @@
> +    _dbus_assert_not_reached ("Didn't find 'l'");
> +  _dbus_assert (i == 2);
> +
> +  if (_dbus_string_find_char_from_sorted_array(&str, 2, _dbus_string_get_length (&str) - 1, "cem", &i))
> +    _dbus_assert_not_reached ("Did find 'e'");

ditto.

@@ +708,5 @@
> +
> +  if (_dbus_string_find_char_from_sorted_array(&str, 2, _dbus_string_get_length (&str) - 1, "cem", &i))
> +    _dbus_assert_not_reached ("Did find 'e'");
> +  _dbus_assert (i == -1);
> +

In fact, the above test cases are basically two different cases. We need some more like: find single char, escape chars, up lower chars and etc.

::: dbus/dbus-string.c
@@ +2058,5 @@
> +  /* '\0' is found in every string */
> +  if (*chars == '\0')
> +    {
> +      if (found)
> +        *found = start;

why not the last char of string? which is the real '\0'. Or just do _dbus_assert (*chars != '\0');

@@ +2067,5 @@
> +
> +#ifndef DBUS_DISABLE_ASSERT
> +  for ( i = 0; i < last_char_pos; i++ )
> +    {
> +       _dbus_assert (chars[i] < chars[i + 1]);

this is the core part of this function, should not depends on dbus assert. Needs to be ensure when assertion disabled.

Comment on attachment 25119 [details] [review]
Improved _dbus_keyring_validate_context() and introduction of _dbus_string_find_char_from_sorted_array()

Review of attachment 25119 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-string.c
@@ +2027,5 @@
> + * Finds a char from an array sorted in ASCII order
> + * in the string, starting from a certain position
> + * up to another.
> + * Returns #TRUE and filling in the byte index
> + * where one of the char was found, if it was found.

", if it was found" seems redundant.

@@ +2029,5 @@
> + * up to another.
> + * Returns #TRUE and filling in the byte index
> + * where one of the char was found, if it was found.
> + * Returns #FALSE if any char is found.
> + * Sets *found to -1 if none of the chars is found

Returns #FALSE if none of chars was found and sets *found to -1 if its not NULL pointer.

What's wrong with the current implementation? If it isn't broken, we shouldn't fix it.

The patch does delete this comment

- * @todo this is the most inefficient implementation
- * imaginable.

but if it isn't invoked frequently enough that its performance matters, then I don't really care. Let's not optimize things that don't show up in benchmarks; our frequently-used code is bad enough (e.g. Bug #38288).