As requested by Willy and Thomas[1], here go some more fixes and tests for
strlcpy() and strlcat().
From the previous discussion, I uderstand that we want to apply the first patch
(export strlen()) as is, I've included it again here just in case. Maybe we should just include the
-Wl,--gc-sections in the nolibc.h comment about the flags to use when compiling?
The rest of the commits are quite simple too, they just:
* Fix the return code of both functions
* Make sure to always null-terminate the dst buffer
* Don't copy more than what size allows us (this handles the size=0 case
for free too)
All has been checked against the corresponding libbsd implementation[2].
I thought the manpage was clear, but when checking against that, I noted a few
twists (like the manpage says the return code of strlcat is strlen(src) +
strlen(dst), but it was not clear it is not that if size < strlen(dst). When
looking at the libbsd implementation and re-reading the manpage, I understood
what it really meant).
Let me know what you think :)
Best,
Rodrigo
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://gitlab.freedesktop.org/libbsd/libbsd.git
Rodrigo Campos (4):
tools/nolibc/string: export strlen()
tools/nolibc: Fix strlcat() return code and size usage
tools/nolibc: Fix strlcpy() return code and size usage
selftests/nolibc: Add tests for strlcat() and strlcpy()
tools/include/nolibc/string.h | 25 +++++++-------
tools/testing/selftests/nolibc/nolibc-test.c | 34 ++++++++++++++++++++
2 files changed, 47 insertions(+), 12 deletions(-)
--
2.43.0
I've verified that the tests matches libbsd's strlcat()/strlcpy()
implementation.
Please note that as strlcat()/strlcpy() are not part of the libc, the
tests are only compiled when using nolibc.
Signed-off-by: Rodrigo Campos <[email protected]>
---
tools/testing/selftests/nolibc/nolibc-test.c | 34 ++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 6ba4f8275ac4..aa365443bb2b 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -600,6 +600,25 @@ int expect_strne(const char *expr, int llen, const char *cmp)
return ret;
}
+#define EXPECT_STRBUFEQ(cond, expr, buf, val, cmp) \
+ do { if (!(cond)) result(llen, SKIPPED); else ret += expect_str_buf_eq(expr, buf, val, llen, cmp); } while (0)
+
+static __attribute__((unused))
+int expect_str_buf_eq(size_t expr, const char *buf, size_t val, int llen, const char *cmp)
+{
+ llen += printf(" = %lu <%s> ", expr, buf);
+ if (strcmp(buf, cmp) != 0) {
+ result(llen, FAIL);
+ return 1;
+ }
+ if (expr != val) {
+ result(llen, FAIL);
+ return 1;
+ }
+
+ result(llen, OK);
+ return 0;
+}
/* declare tests based on line numbers. There must be exactly one test per line. */
#define CASE_TEST(name) \
@@ -991,6 +1010,9 @@ int run_stdlib(int min, int max)
for (test = min; test >= 0 && test <= max; test++) {
int llen = 0; /* line length */
+ /* For functions that take a long buffer, like strlcat() */
+ char buf[7] = "foo";
+
/* avoid leaving empty lines below, this will insert holes into
* test numbers.
*/
@@ -1007,6 +1029,18 @@ int run_stdlib(int min, int max)
CASE_TEST(strchr_foobar_z); EXPECT_STRZR(1, strchr("foobar", 'z')); break;
CASE_TEST(strrchr_foobar_o); EXPECT_STREQ(1, strrchr("foobar", 'o'), "obar"); break;
CASE_TEST(strrchr_foobar_z); EXPECT_STRZR(1, strrchr("foobar", 'z')); break;
+#ifdef NOLIBC
+ CASE_TEST(strlcat_0); EXPECT_STRBUFEQ(1, strlcat(buf, "bar", 0), buf, 3, "foo"); break;
+ CASE_TEST(strlcat_1); EXPECT_STRBUFEQ(1, strlcat(buf, "bar", 1), buf, 4, "foo"); break;
+ CASE_TEST(strlcat_3); EXPECT_STRBUFEQ(1, strlcat(buf, "bar", 3), buf, 6, "foo"); break;
+ CASE_TEST(strlcat_5); EXPECT_STRBUFEQ(1, strlcat(buf, "bar", 5), buf, 6, "foob"); break;
+ CASE_TEST(strlcat_7); EXPECT_STRBUFEQ(1, strlcat(buf, "bar", 7), buf, 6, "foobar"); break;
+ CASE_TEST(strlcpy_0); EXPECT_STRBUFEQ(1, strlcpy(buf, "bar", 0), buf, 3, "foo"); break;
+ CASE_TEST(strlcpy_1); EXPECT_STRBUFEQ(1, strlcpy(buf, "bar", 1), buf, 3, ""); break;
+ CASE_TEST(strlcpy_2); EXPECT_STRBUFEQ(1, strlcpy(buf, "bar", 2), buf, 3, "b"); break;
+ CASE_TEST(strlcpy_3); EXPECT_STRBUFEQ(1, strlcpy(buf, "bar", 3), buf, 3, "ba"); break;
+ CASE_TEST(strlcpy_4); EXPECT_STRBUFEQ(1, strlcpy(buf, "bar", 4), buf, 3, "bar"); break;
+#endif
CASE_TEST(memcmp_20_20); EXPECT_EQ(1, memcmp("aaa\x20", "aaa\x20", 4), 0); break;
CASE_TEST(memcmp_20_60); EXPECT_LT(1, memcmp("aaa\x20", "aaa\x60", 4), 0); break;
CASE_TEST(memcmp_60_20); EXPECT_GT(1, memcmp("aaa\x60", "aaa\x20", 4), 0); break;
--
2.43.0
The return code should always be strlen(src), and we should copy at most
size-1 bytes.
While we are there, make sure to null-terminate the dst buffer.
Signed-off-by: Rodrigo Campos <[email protected]>
---
tools/include/nolibc/string.h | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
index b2149e1342a8..e4bc0302967d 100644
--- a/tools/include/nolibc/string.h
+++ b/tools/include/nolibc/string.h
@@ -212,15 +212,16 @@ size_t strlcpy(char *dst, const char *src, size_t size)
size_t len;
char c;
- for (len = 0;;) {
+ for (len = 0; len < size; len++) {
c = src[len];
- if (len < size)
+ if (len < size - 1)
dst[len] = c;
+ if (len == size - 1)
+ dst[len] = '\0';
if (!c)
break;
- len++;
}
- return len;
+ return strlen(src);
}
static __attribute__((unused))
--
2.43.0
Hi Rodrigo,
On Mon, Jan 29, 2024 at 03:15:15PM +0100, Rodrigo Campos wrote:
> The return code should always be strlen(src), and we should copy at most
> size-1 bytes.
>
> While we are there, make sure to null-terminate the dst buffer.
>
> Signed-off-by: Rodrigo Campos <[email protected]>
> ---
> tools/include/nolibc/string.h | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tools/include/nolibc/string.h b/tools/include/nolibc/string.h
> index b2149e1342a8..e4bc0302967d 100644
> --- a/tools/include/nolibc/string.h
> +++ b/tools/include/nolibc/string.h
> @@ -212,15 +212,16 @@ size_t strlcpy(char *dst, const char *src, size_t size)
> size_t len;
> char c;
>
> - for (len = 0;;) {
> + for (len = 0; len < size; len++) {
> c = src[len];
> - if (len < size)
> + if (len < size - 1)
> dst[len] = c;
> + if (len == size - 1)
> + dst[len] = '\0';
> if (!c)
> break;
> - len++;
> }
> - return len;
> + return strlen(src);
> }
It's good, but for the same reason as the previous one, I'm getting
smaller code by doing less in the loop. Also calling strlen() here
looks expensive, I'm seeing that the compiler inlined it nevertheless
and did it in a dep-optimized way due to the asm statement. That
results in 67 bytes total while a simpler version gives 47.
If I explicitly mark strlen() __attribute__((noinline)) that prevents
it from doing so starting with gcc-10, where it correctly places a jump
from strlcpy() to strlen() and ends up with 50 bytes (vs 44 for the alt
one). The other one I can propose is directly derived from the other
strlcat() variant, which first performs the copy and starts to count:
size_t strlcpy(char *dst, const char *src, size_t size)
{
size_t len;
for (len = 0; len < size; len++) {
if (!(dst[len] = src[len]))
return len;
}
/* end of src not found before size */
if (size)
dst[size - 1] = '\0';
while (src[len])
len++;
return len;
}
Just let me know what you think. And I think we should explicitly mark
strlen() and the few other ones we're marking weak as noinline so that
the compiler perfers a call there to inlining. Well, registers clobbering
might not always be worth, but given that strlen() alone provides some
savings I think it's still interesting.
Thanks,
Willy
On Mon, Jan 29, 2024 at 03:15:16PM +0100, Rodrigo Campos wrote:
> I've verified that the tests matches libbsd's strlcat()/strlcpy()
> implementation.
>
> Please note that as strlcat()/strlcpy() are not part of the libc, the
> tests are only compiled when using nolibc.
>
> Signed-off-by: Rodrigo Campos <[email protected]>
I like it, simple and efficient, thank you!
Willy
On Sun, Feb 11, 2024 at 12:08:14PM +0100, Willy Tarreau wrote:
> And I think we should explicitly mark
> strlen() and the few other ones we're marking weak as noinline so that
> the compiler perfers a call there to inlining.
So actually the weak argument always prevents inlining from happening
so this is not needed (I didn't have it in my previous test).
Willy
On 2/11/24 12:08, Willy Tarreau wrote:
> Hi Rodrigo,
>
> It's good, but for the same reason as the previous one, I'm getting
> smaller code by doing less in the loop. Also calling strlen() here
> looks expensive, I'm seeing that the compiler inlined it nevertheless
> and did it in a dep-optimized way due to the asm statement. That
> results in 67 bytes total while a simpler version gives 47.
>
> If I explicitly mark strlen() __attribute__((noinline)) that prevents
> it from doing so starting with gcc-10, where it correctly places a jump
> from strlcpy() to strlen() and ends up with 50 bytes (vs 44 for the alt
> one). The other one I can propose is directly derived from the other
> strlcat() variant, which first performs the copy and starts to count:
>
> size_t strlcpy(char *dst, const char *src, size_t size)
> {
> size_t len;
>
> for (len = 0; len < size; len++) {
> if (!(dst[len] = src[len]))
> return len;
> }
>
> /* end of src not found before size */
> if (size)
> dst[size - 1] = '\0';
>
> while (src[len])
> len++;
>
> return len;
> }
>
> Just let me know what you think.
This is one is very nice, thanks!
Sorry I didn't think about the size at all when writing the functions :)
We can change the loop to be:
for (len = 0; len < size; len++) {
dst[len] = src[len];
if (!dst[len])
break;
}
That IMHO it is slightly more readable and makes it only 2 bytes longer
here.
What do you think? I'm fine with both, of course.
If I resend, shall I add a suggested-by or directly you as the author?
Best,
Rodrigo
On Wed, Feb 14, 2024 at 12:50:53PM -0300, Rodrigo Campos wrote:
> On 2/11/24 12:08, Willy Tarreau wrote:
> > Hi Rodrigo,
> >
> > It's good, but for the same reason as the previous one, I'm getting
> > smaller code by doing less in the loop. Also calling strlen() here
> > looks expensive, I'm seeing that the compiler inlined it nevertheless
> > and did it in a dep-optimized way due to the asm statement. That
> > results in 67 bytes total while a simpler version gives 47.
> >
> > If I explicitly mark strlen() __attribute__((noinline)) that prevents
> > it from doing so starting with gcc-10, where it correctly places a jump
> > from strlcpy() to strlen() and ends up with 50 bytes (vs 44 for the alt
> > one). The other one I can propose is directly derived from the other
> > strlcat() variant, which first performs the copy and starts to count:
> >
> > size_t strlcpy(char *dst, const char *src, size_t size)
> > {
> > size_t len;
> >
> > for (len = 0; len < size; len++) {
> > if (!(dst[len] = src[len]))
> > return len;
> > }
> >
> > /* end of src not found before size */
> > if (size)
> > dst[size - 1] = '\0';
> >
> > while (src[len])
> > len++;
> >
> > return len;
> > }
> >
> > Just let me know what you think.
>
> This is one is very nice, thanks!
>
> Sorry I didn't think about the size at all when writing the functions :)
Never be sorry, low-level user code like this is never trivial and
that's the goal of the nolibc-test in the first place ;-)
> We can change the loop to be:
>
> for (len = 0; len < size; len++) {
> dst[len] = src[len];
> if (!dst[len])
> break;
> }
>
> That IMHO it is slightly more readable and makes it only 2 bytes longer
> here.
It's not exactly the same, it will always write a zero at dst[size-1]
due to the break statement. As much as I hate returns in the middle,
this one made sense for this case. A goto to the final return statement
is fine as well.
> What do you think? I'm fine with both, of course.
I'm fine with the more readable part (I also prefer it) but not the use
of break here.
> If I resend, shall I add a suggested-by or directly you as the author?
No need for either, it's your work, my part was just a review and an
addictive temptation to look at asm code ;-)
Cheers,
Willy
On 1/29/24 15:15, Rodrigo Campos wrote:
> + /* For functions that take a long buffer, like strlcat() */
> + char buf[7] = "foo";
> +
I've improved this to not be the same length as the other buffer (we
can't catch mistakes like confusing the src and dst buffers like that)
and added some trailing chars, so we also test that the \0 is added at
the exactly right position.