2022-03-21 14:14:55

by Ammar Faizi

[permalink] [raw]
Subject: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

Add strdup and strndup support. These functions are only available on
architectures that have my_syscall6() macro from nolibc.

Signed-off-by: Ammar Faizi <[email protected]>
---
tools/include/nolibc/string.h | 68 +++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)

diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
index 4554b6fcb400..413c65f7c853 100644
--- a/tools/include/nolibc/string.h
+++ b/tools/include/nolibc/string.h
@@ -9,6 +9,10 @@

#include "std.h"

+static void free(void *ptr);
+static void *malloc(size_t len);
+static void *realloc(void *old_ptr, size_t new_size);
+
/*
* As much as possible, please keep functions alphabetically sorted.
*/
@@ -127,6 +131,70 @@ size_t nolibc_strlen(const char *str)
nolibc_strlen((str)); \
})

+static __attribute__((unused))
+char *strdup(const char *str)
+{
+ size_t allocated = 2048;
+ size_t i;
+ char *ret;
+ char *tmp;
+
+ ret = malloc(allocated);
+ if (__builtin_expect(!ret, 0))
+ return NULL;
+
+ i = 0;
+ for (;;) {
+ char c = *str;
+ if (!c)
+ break;
+
+ if (i == allocated) {
+ allocated += 2048;
+ tmp = realloc(ret, allocated);
+ if (__builtin_expect(!tmp, 0)) {
+ free(ret);
+ return NULL;
+ }
+ ret = tmp;
+ }
+
+ ret[i++] = c;
+ str++;
+ }
+
+ ret[i] = '\0';
+ return ret;
+}
+
+static __attribute__((unused))
+char *strndup(const char *str, size_t maxlen)
+{
+ size_t i;
+ char *ret;
+
+ ret = malloc(maxlen + 1);
+ if (__builtin_expect(!ret, 0))
+ return NULL;
+
+ i = 0;
+ for (;;) {
+ char c = *str;
+ if (!c)
+ break;
+
+ if (i == maxlen)
+ break;
+
+ ret[i++] = c;
+ str++;
+ }
+
+ ret[i] = '\0';
+ return ret;
+}
+
+
static __attribute__((unused))
size_t strlcat(char *dst, const char *src, size_t size)
{
--
Ammar Faizi


2022-03-21 17:52:15

by Alviro Iskandar Setiawan

[permalink] [raw]
Subject: Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
> +}
> +
> +

(Trivial) Got double newlines here, one newline should be good.

> static __attribute__((unused))
> size_t strlcat(char *dst, const char *src, size_t size)
> {

-- Viro

2022-03-21 20:59:04

by Willy Tarreau

[permalink] [raw]
Subject: Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

On Mon, Mar 21, 2022 at 06:36:37PM +0700, Ammar Faizi wrote:
> On 3/21/22 2:53 PM, Willy Tarreau wrote:
> > Hi Ammar,
> [...]
> > > +static void free(void *ptr);
> > > +static void *malloc(size_t len);
> > > +static void *realloc(void *old_ptr, size_t new_size);
> >
> > Better include the required h files here.
>
> I can't do that, in nolibc.h, we have something like this:
>
> ```
> #include "stdlib.h" <--- We inlcude string.h from here
> #include "string.h" <--- This is a no-op.
> ```
>
> Note, stdlib.h is included first before string.h, next, in stdlib.h, we
> have this:
>
> ```
> #include "string.h"
>
> // malloc, calloc, free here
> ```
>
> If I include "stdlib.h" in "string.h", it will just be a no-op, and the
> declarations will not be taken, because the declarations happen after
> #include "string.h". So it doesn't work. It's somewhat circular dependency.
>
> stdlib.h needs string.h
> string.h needs stdlib.h
>
> One of them must fully see the other before they can use the defined functions
> in another header.

OK, usual stuff indeed.

> Suggestion welcome...

Then just leave it as-is.

> I am thinking of creating a new header just for the forward declarations
> where all function declarations live there, we just split off the real
> functions' body.

That's why I'm doing in my projects for the exact reason above. But
here we only have static functions so this will increase the burden to
contribute. Better wait for the situation to reach a point where we're
certain there will be some benefit in doing that.

Thanks,
Willy

2022-03-21 21:29:57

by Ammar Faizi

[permalink] [raw]
Subject: Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

On 3/21/22 2:53 PM, Willy Tarreau wrote:
> Hi Ammar,
[...]
>> +static void free(void *ptr);
>> +static void *malloc(size_t len);
>> +static void *realloc(void *old_ptr, size_t new_size);
>
> Better include the required h files here.

I can't do that, in nolibc.h, we have something like this:

```
#include "stdlib.h" <--- We inlcude string.h from here
#include "string.h" <--- This is a no-op.
```

Note, stdlib.h is included first before string.h, next, in stdlib.h, we
have this:

```
#include "string.h"

// malloc, calloc, free here
```

If I include "stdlib.h" in "string.h", it will just be a no-op, and the
declarations will not be taken, because the declarations happen after
#include "string.h". So it doesn't work. It's somewhat circular dependency.

stdlib.h needs string.h
string.h needs stdlib.h

One of them must fully see the other before they can use the defined functions
in another header.

Suggestion welcome...

I am thinking of creating a new header just for the forward declarations
where all function declarations live there, we just split off the real
functions' body.

[...]
>
> This version is suboptimal in terms of code size, CPU usage and memory
> usage. And it even seems it contains a buffer overflow: if the string
> is exactly a multiple of 2048, it seems to me that you'll write the
> trailing zero past the end. Please instead use the more intuitive form
> below (not tested but you get the idea):
>
> size_t len = strlen(str);
> char *ret = malloc(len + 1);
> if (ret)
> memcpy(ret, str, len);
> return ret;

Ah right, that's indeed overflow. Will fold this in as you suggested.

[...]
> Here it can cost quite a lot for large values of maxlen. Please just use
> a variant of the proposal above like this one:
>
> size_t len;
> char *ret;
>
> len = strlen(str);
> if (len > maxlen)
> len = maxlen;
> ret = malloc(len + 1);
> if (ret)
> memcpy(ret, str, len);
> return ret;

Will take the Alviro's suggestion for this part...

--
Ammar Faizi

2022-03-21 22:03:28

by Ammar Faizi

[permalink] [raw]
Subject: Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

On 3/20/22 10:55 PM, Alviro Iskandar Setiawan wrote:
> On Sun, Mar 20, 2022 at 4:37 PM Ammar Faizi wrote:
>> +}
>> +
>> +
>
> (Trivial) Got double newlines here, one newline should be good.

OK, fixed.

>> static __attribute__((unused))
>> size_t strlcat(char *dst, const char *src, size_t size)
>> {

--
Ammar Faizi

2022-03-21 22:15:09

by Willy Tarreau

[permalink] [raw]
Subject: Re: [RFC PATCH v1 6/6] tools/include/string: Implement `strdup()` and `strndup()`

Hi Ammar,

On Sun, Mar 20, 2022 at 04:37:50PM +0700, Ammar Faizi wrote:
> Add strdup and strndup support. These functions are only available on
> architectures that have my_syscall6() macro from nolibc.
>
> Signed-off-by: Ammar Faizi <[email protected]>
> ---
> tools/include/nolibc/string.h | 68 +++++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
> index 4554b6fcb400..413c65f7c853 100644
> --- a/tools/include/nolibc/string.h
> +++ b/tools/include/nolibc/string.h
> @@ -9,6 +9,10 @@
>
> #include "std.h"
>
> +static void free(void *ptr);
> +static void *malloc(size_t len);
> +static void *realloc(void *old_ptr, size_t new_size);

Better include the required h files here.

> /*
> * As much as possible, please keep functions alphabetically sorted.
> */
> @@ -127,6 +131,70 @@ size_t nolibc_strlen(const char *str)
> nolibc_strlen((str)); \
> })
>
> +static __attribute__((unused))
> +char *strdup(const char *str)
> +{
> + size_t allocated = 2048;
> + size_t i;
> + char *ret;
> + char *tmp;
> +
> + ret = malloc(allocated);
> + if (__builtin_expect(!ret, 0))
> + return NULL;
> +
> + i = 0;
> + for (;;) {
> + char c = *str;
> + if (!c)
> + break;
> +
> + if (i == allocated) {
> + allocated += 2048;
> + tmp = realloc(ret, allocated);
> + if (__builtin_expect(!tmp, 0)) {
> + free(ret);
> + return NULL;
> + }
> + ret = tmp;
> + }
> +
> + ret[i++] = c;
> + str++;
> + }
> +
> + ret[i] = '\0';
> + return ret;
> +}

This version is suboptimal in terms of code size, CPU usage and memory
usage. And it even seems it contains a buffer overflow: if the string
is exactly a multiple of 2048, it seems to me that you'll write the
trailing zero past the end. Please instead use the more intuitive form
below (not tested but you get the idea):

size_t len = strlen(str);
char *ret = malloc(len + 1);
if (ret)
memcpy(ret, str, len);
return ret;

> +static __attribute__((unused))
> +char *strndup(const char *str, size_t maxlen)
> +{
> + size_t i;
> + char *ret;
> +
> + ret = malloc(maxlen + 1);
> + if (__builtin_expect(!ret, 0))
> + return NULL;
> +
> + i = 0;
> + for (;;) {
> + char c = *str;
> + if (!c)
> + break;
> +
> + if (i == maxlen)
> + break;
> +
> + ret[i++] = c;
> + str++;
> + }
> +
> + ret[i] = '\0';
> + return ret;
> +}

Here it can cost quite a lot for large values of maxlen. Please just use
a variant of the proposal above like this one:

size_t len;
char *ret;

len = strlen(str);
if (len > maxlen)
len = maxlen;
ret = malloc(len + 1);
if (ret)
memcpy(ret, str, len);
return ret;

Thanks,
Willy