[PATCH] string: strtok, strtok_r should accept initial NULL subject (bug 16640)

[PATCH] string: strtok, strtok_r should accept initial NULL subject (bug 16640)

[Florian Weimer]

The BSD and musl implementations accept an initial NULL subject
argument.  This also used to be support in glibc on some architectures
with custom assembler code.

Tested on i686-linux-gnu and x86_64-linux-gnu.

---
 string/Makefile          |  1 +
 string/strtok_r.c        |  6 +++++-
 string/tst-strtok-null.c | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/string/Makefile b/string/Makefile
index 938f528b8d..a1c6587376 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -177,6 +177,7 @@ tests := \
   tst-strfry \
   tst-strlen \
   tst-strtok \
+  tst-strtok-null \
   tst-strtok_r \
   tst-strxfrm \
   tst-strxfrm2 \
diff --git a/string/strtok_r.c b/string/strtok_r.c
index fd3a842c99..8342d2ac74 100644
--- a/string/strtok_r.c
+++ b/string/strtok_r.c
@@ -44,7 +44,11 @@ __strtok_r (char *s, const char *delim, char **save_ptr)
   char *end;
 
   if (s == NULL)
-    s = *save_ptr;
+    {
+      if (*save_ptr == NULL)
+	return NULL;
+      s = *save_ptr;
+    }
 
   if (*s == '\0')
     {
diff --git a/string/tst-strtok-null.c b/string/tst-strtok-null.c
new file mode 100644
index 0000000000..2cbc4e5fc4
--- /dev/null
+++ b/string/tst-strtok-null.c
@@ -0,0 +1,32 @@
+/* Check that strtok and strtok_r accept NULL for the initial subject string.
+   Copyright (C) 2022 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stddef.h>
+#include <string.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  TEST_COMPARE_STRING (strtok (NULL, ","), NULL);
+  char *save = NULL;
+  TEST_COMPARE_STRING (strtok_r (NULL, ",", &save), NULL);
+  return 0;
+}
+
+#include <support/test-driver.c>

base-commit: 9cc9d61ee12f2f8620d8e0ea3c42af02bf07fe1e

Re: [PATCH] string: strtok, strtok_r should accept initial NULL subject (bug 16640)

[Carlos O'Donell]

On 11/7/22 03:55, Florian Weimer via Libc-alpha wrote:
> The BSD and musl implementations accept an initial NULL subject
> argument.  This also used to be support in glibc on some architectures
> with custom assembler code.

(1) Standards

The ISO C standard pretty clearly says that the first call in the sequence has a
non-null first argument.

(2) Security

From a security stand point I do not want to see a change in behaviour where we
go from failing quickly and immediately at the point we invoke undefined behaviour
to potentially failing far from the point.

We continue to move in this direction across all glibc APIs and we have previously
discussed that we don't want to run with a potentially invalid state for any longer
than required to shut the process down, and the shutdown itself should be as quick
as possible.

(3) Performance/Stability

Terminating the process quickly reduces restart latency, which means the service
can be restarted quickly when an error is encountered.

(4) Compatibility

I don't see any strong justification for why we would change the behaviour, other
than compatibility with musl or certain BSDs. The compatibility of a undefined
behaviour, poorly defined behaviour or implementation defined behaviour needs to
be evaluated on the merits of the individual change. In this case I don't see a
strong justification for exact compatibility in the "first call first argument NULL"
case. I see a strong argument for harmonizing the behaviour and we did that in
the glibc 2.26 release.

Note that this behaviour is not implementation defined, but such behaviour would
need to be documented. This is undefined behaviour.

(5) Existing behaviour.

The glibc implementation behaviour was not uniform across all architectures, and
we fixed that in glibc 2.26.  We have had this behaviour in place for 4 years
across all the architectures, and even longer in the generic code.

In summary:

- I don't see a strong justification to change glibc. This gets a NACK from me.
 
> Tested on i686-linux-gnu and x86_64-linux-gnu.
> 
> ---
>  string/Makefile          |  1 +
>  string/strtok_r.c        |  6 +++++-
>  string/tst-strtok-null.c | 32 ++++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/string/Makefile b/string/Makefile
> index 938f528b8d..a1c6587376 100644
> --- a/string/Makefile
> +++ b/string/Makefile
> @@ -177,6 +177,7 @@ tests := \
>    tst-strfry \
>    tst-strlen \
>    tst-strtok \
> +  tst-strtok-null \
>    tst-strtok_r \
>    tst-strxfrm \
>    tst-strxfrm2 \
> diff --git a/string/strtok_r.c b/string/strtok_r.c
> index fd3a842c99..8342d2ac74 100644
> --- a/string/strtok_r.c
> +++ b/string/strtok_r.c
> @@ -44,7 +44,11 @@ __strtok_r (char *s, const char *delim, char **save_ptr)
>    char *end;
>  
>    if (s == NULL)
> -    s = *save_ptr;
> +    {
> +      if (*save_ptr == NULL)
> +	return NULL;
> +      s = *save_ptr;
> +    }
>  
>    if (*s == '\0')
>      {
> diff --git a/string/tst-strtok-null.c b/string/tst-strtok-null.c
> new file mode 100644
> index 0000000000..2cbc4e5fc4
> --- /dev/null
> +++ b/string/tst-strtok-null.c
> @@ -0,0 +1,32 @@
> +/* Check that strtok and strtok_r accept NULL for the initial subject string.
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stddef.h>
> +#include <string.h>
> +#include <support/check.h>
> +
> +static int
> +do_test (void)
> +{
> +  TEST_COMPARE_STRING (strtok (NULL, ","), NULL);
> +  char *save = NULL;
> +  TEST_COMPARE_STRING (strtok_r (NULL, ",", &save), NULL);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> 
> base-commit: 9cc9d61ee12f2f8620d8e0ea3c42af02bf07fe1e
> 

-- 
Cheers,
Carlos.