Change in coreboot[master]: src/lib/string.c: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: src/lib/string.c: Add strtok() and strtok_r()
......................................................................

src/lib/string.c: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/include/string.h
M src/lib/string.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/1

diff --git a/src/include/string.h b/src/include/string.h
index f923ca5..9356e3b 100644
--- a/src/include/string.h
+++ b/src/include/string.h
@@ -30,6 +30,8 @@
 int strspn(const char *str, const char *spn);
 int strcspn(const char *str, const char *spn);
 long atol(const char *str);
+char *strtok(char *str, const char *delim);
+char *strtok_r(char *str, const char *delim, char **ptr);

/**
  * Find a character in a string.
diff --git a/src/lib/string.c b/src/lib/string.c
index f0c24ed..7bec606 100644
--- a/src/lib/string.c
+++ b/src/lib/string.c
@@ -5,6 +5,8 @@
 #include <stddef.h>
 #include <stdlib.h>

+static char **strtok_global;
+
 char *strdup(const char *s)
 {
    if (!ENV_RAMSTAGE)
@@ -184,3 +186,23 @@
    }
    return ret * sign;
 }
+
+char *strtok_r(char *str, const char *delim, char **ptr)
+{
+	if (str == NULL)
+		str = *ptr;
+	char *start = str + strspn(str, delim);
+	if (start[0] == '\0')
+		return NULL;
+
+	char *end = start + strcspn(start, delim);
+	*ptr = end;
+	if (end[0] != '\0')
+		*(*ptr)++ = '\0';
+	return start;
+}
+
+char *strtok(char *str, const char *delim)
+{
+	return strtok_r(str, delim, strtok_global);
+}

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: src/lib/string.c: Add strtok() and strtok_r()
......................................................................

Patch Set 1: Code-Review+1

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41420

to look at the new patch set (#3).

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/include/string.h
M src/lib/string.c
2 files changed, 24 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/3

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 3: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@194 
PS3, Line 194: char *start
Try to define all needed internal variables at the beginning of the function if they will be used unconditionally inside the function (which would be *start and *end).

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@207 
PS3, Line 207: strtok_global
Would it make sense to move strtok_global into this function and declaring it as static?
And maybe give it a defferent name like strtok_ptr or int_ptr?

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41420

to look at the new patch set (#4).

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/drivers/ipmi/ipmi_kcs_ops.c
M src/include/string.h
M src/lib/string.c
3 files changed, 29 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/4

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41420

to look at the new patch set (#5).

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/include/string.h
M src/lib/string.c
2 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/5

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@194 
PS3, Line 194: char *start
> Try to define all needed internal variables at the beginning of the function if they will be used un […]

Done

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@207 
PS3, Line 207: strtok_global
> Would it make sense to move strtok_global into this function and declaring it as static? […]

Done

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 5:

(2 comments)

Aren't you dereferencing a NULL pointer here?

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@8 
PS3, Line 8: strtok_global
*strtok_global

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@207 
PS3, Line 207: strtok_global
&strtok_global

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 5:
> Patch Set 5:
>
> (2 comments)
>
> Aren't you dereferencing a NULL pointer here?

Yes it should be &strtok_ptr.

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41420

to look at the new patch set (#6).

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/include/string.h
M src/lib/string.c
2 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/6

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Sindhoor Tilak (Code Review)]

Sindhoor Tilak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(1 comment)

Shouldn't this be a single dereference?

https://review.coreboot.org/c/coreboot/+/41420/6/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/6/src/lib/string.c@202 
PS6, Line 202: 		*(*ptr)++ = '\0';
*ptr

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:
> Patch Set 6:
>
> (1 comment)
>
> Shouldn't this be a single dereference?

No. That's correct. First,'\0' is put at the location where delimiter was found (**ptr = '\0'). Then, 'end' is moved so that it points to the very next location (*ptr++).

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@8 
PS3, Line 8: strtok_global
> *strtok_global

Am I missing something or is this also a serious bug in libpayload? Did this ever work or is it just that nobody ever used this? (I guess strsep() is just the generally more popular variant? Should we even add strtok() to coreboot then?)

Can someone please fix this in libpayload too?

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@194 
PS3, Line 194: char *start
> Done

Just want to add that as far as I know, we have no rule against C99 declarations in coreboot. I use them all the time because I think they tend to be easier to read. Doesn't mean there's anything wrong with writing this function with C90 declarations, I think we just generally leave that up to the author.

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@194 
PS3, Line 194: char *start
> Just want to add that as far as I know, we have no rule against C99 declarations in coreboot. […]

Yes, you are right Julius.
I still have this C90 style deep in my bones. But yes, there is no written rule or the like.

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@8 
PS3, Line 8: strtok_global
> Am I missing something or is this also a serious bug in libpayload? Did this ever work or is it just […]

I just pushed a patch to fix this in libpayload.

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@194 
PS3, Line 194: char *start
> Yes, you are right Julius. […]

Ack

https://review.coreboot.org/c/coreboot/+/41420/3/src/lib/string.c@207 
PS3, Line 207: strtok_global
> &strtok_global

Done

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/6/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/6/src/lib/string.c@202 
PS6, Line 202: 		*(*ptr)++ = '\0';
> *ptr

Hey Harshit. You still have one unresolved issue here. Can you please take a look so that we can bring this patch in?

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/6/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/6/src/lib/string.c@202 
PS6, Line 202: 		*(*ptr)++ = '\0';
> > *ptr […]

Sure. 
Actually I've clarified this already. Let me mark it resolved.

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/6/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/c/coreboot/+/41420/6/src/include/string.h@34 
PS6, Line 34: char *strtok_r(char *str, const char *delim, char **ptr);
Add them above `atol()` to group them better? But not important.

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Sindhoor Tilak, Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41420

to look at the new patch set (#7).

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/include/string.h
M src/lib/string.c
2 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/7

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/6/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/c/coreboot/+/41420/6/src/include/string.h@34 
PS6, Line 34: char *strtok_r(char *str, const char *delim, char **ptr);
> Add them above `atol()` to group them better? But not important.

Done

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/7/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/c/coreboot/+/41420/7/src/include/string.h@32 
PS7, Line 32: char *strtok(char *str, const char *delim);
            : char *strtok_r(char *str, const char *delim, char **ptr);
Once you have changed the order in the header file you should do it in the c file as well so that they match again. I guess this is what Paul was trying to say.

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Hello build bot (Jenkins), Sindhoor Tilak, Paul Menzel, Julius Werner, Werner Zeh, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41420

to look at the new patch set (#8).

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
---
M src/include/string.h
M src/lib/string.c
2 files changed, 27 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/41420/8

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Harshit Sharma (Code Review)]

Harshit Sharma has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/7/src/include/string.h 
File src/include/string.h:

https://review.coreboot.org/c/coreboot/+/41420/7/src/include/string.h@32 
PS7, Line 32: char *strtok(char *str, const char *delim);
            : char *strtok_r(char *str, const char *delim, char **ptr);
> Once you have changed the order in the header file you should do it in the c file as well so that th […]

Done

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

lib: Add strtok() and strtok_r()

Add strtok() and strtok_r() to the library.

Signed-off-by: Harshit Sharma 
Change-Id: Ic855b31669be1c274cbf247c53ffa6f74ec5bf35
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41420
Tested-by: build bot (Jenkins) 
Reviewed-by: Werner Zeh 
---
M src/include/string.h
M src/lib/string.c
2 files changed, 27 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Werner Zeh: Looks good to me, approved

diff --git a/src/include/string.h b/src/include/string.h
index f923ca5..8eef068 100644
--- a/src/include/string.h
+++ b/src/include/string.h
@@ -29,6 +29,8 @@
 int strncmp(const char *s1, const char *s2, int maxlen);
 int strspn(const char *str, const char *spn);
 int strcspn(const char *str, const char *spn);
+char *strtok_r(char *str, const char *delim, char **ptr);
+char *strtok(char *str, const char *delim);
 long atol(const char *str);

/**
diff --git a/src/lib/string.c b/src/lib/string.c
index f0c24ed..e8f72a2 100644
--- a/src/lib/string.c
+++ b/src/lib/string.c
@@ -163,6 +163,31 @@
    return ret;
 }

+char *strtok_r(char *str, const char *delim, char **ptr)
+{
+	char *start;
+	char *end;
+
+	if (str == NULL)
+		str = *ptr;
+	start = str + strspn(str, delim);
+	if (start[0] == '\0')
+		return NULL;
+
+	end = start + strcspn(start, delim);
+	*ptr = end;
+	if (end[0] != '\0')
+		*(*ptr)++ = '\0';
+	return start;
+}
+
+char *strtok(char *str, const char *delim)
+{
+	static char *strtok_ptr;
+
+	return strtok_r(str, delim, &strtok_ptr);
+}
+
 long atol(const char *str)
 {
    long ret = 0;

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/9/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/9/src/lib/string.c@174 
PS9, Line 174: 	if (start[0] == '\0')
With Debian Sid/unstable and clang-tools-9 1:9.0.1-12 running `scan-build make` for the Lenovo ThinkPad X201, the warning below is shown.

```
    CC         bootblock/lib/string.
src/lib/string.c:174:6: warning: Array access (from variable 'start') results in a null pointer dereference
        if (start[0] == '\0')
            ^~~~~~~~
1 warning generated.
```

Change in coreboot[master]: lib: Add strtok() and strtok_r()

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41420 )

Change subject: lib: Add strtok() and strtok_r()
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41420/9/src/lib/string.c 
File src/lib/string.c:

https://review.coreboot.org/c/coreboot/+/41420/9/src/lib/string.c@174 
PS9, Line 174: 	if (start[0] == '\0')
> With Debian Sid/unstable and clang-tools-9 1:9.0. […]

Seems like a false positive to me. It's not legal to call strtok() with NULL, that's not a bug in our implementation, just how the API is defined. clang probably gets confused because either str or *ptr can be NULL in certain cases, but it doesn't know that it's not legal for both of them to be NULL in the same call.