Re: [PATCH 0/1] tools/nolibc/string: export strlen()

[Willy Tarreau]

Hi!

On Sat, Jan 27, 2024 at 03:53:32PM +0100, Thomas Weißschuh wrote:
> Hi!
> 
> On 2024-01-26 15:24:10+0100, Rodrigo Campos wrote:
> > Hi, while using nolibc on debian testing, I found that compilation fails when using strlcat().
> > 
> > The compilation fails with:
> > 
> >             cc -fno-asynchronous-unwind-tables -fno-ident -s -Os -nostdlib -lgcc  -static -o test test.c
> >             /usr/bin/ld: /tmp/cccIasKL.o: in function `main':
> >             test.c:(.text.startup+0x1e): undefined reference to `strlen'
> >             collect2: error: ld returned 1 exit status
> > 
> > This is using debian testing, with gcc 13.2.0.
> 
> I can reproduce the issue with gcc 13.2.1 on Arch.
> 
> > A small repro case that fails with this error on debian is:
> > 
> > 	int main(void) {
> > 		char dst[6] = "12";
> > 		char *src = "abc";
> > 		strlcat(dst, src, 6);
> > 	
> > 		printf("dst is: %s\n", dst);
> > 	
> > 		return 0;
> > 	}
> > 
> > Please note that this code is not using strlen() and strlcat() doesn't seems to use it either.
> 
> I think the comment in strlen() explains it:
> 
>     Note that gcc12 recognizes an strlen() pattern and replaces it with a
>     jump to strlen().
> 
> strlcat() indeed contains this pattern.
> 
> I was able to fix the issue by replacing the open-coded strlen() in
> strlcat() with a call to the function and that also fixed the issue.
> 
> It seems nicer to me as a fix, on the other hand the change to a weak
> definition will also catch other instances of the issue.
> Maybe we do both.

Yes, once we have the proof that the compiler may produce such a call, it
can also happen in whatever user code so we need to export the function,
there's no other solution.

> > First I noted that removing the attribute unused in strlen(), the compilation worked fine. And then
> > I noticied that other functions had the attribute weak, a custom section and export the function.
> > 
> > In particular, what happens here seems to be the same as in commit "tools/nolibc/string: export memset() and
> > memmove()" (8d304a374023), as removing the -Os or adding the -ffreestanding seem to fix the issue.
> > So, I did the same as that commit, for strlen().
> > 
> > However, I'm not 100% confident on how to check that this is done by the compiler to later replace
> > it and provide a builtin. I'm not sure how that was verified for commit 8d304a374023, but if you let
> > me know, I can verify it too.
> > 
> > What do you think?
> 
> Personally I don't know how it was verified, we'll have to wait for
> Willy on that.

Oh it's very simple, just build a small code that doesn't contain any
such explicit nor implicit call and check that it doesn't contain the
function.

E.g.:

  $ printf "int main(void) { }\n" | gcc -nostdlib -static -Isysroot/x86/include -include nolibc.h -Os -Wl,--gc-sections -xc -
  $ nm --size a.out 
  0000000000000003 T main
  0000000000000004 V errno
  0000000000000008 V _auxv
  0000000000000008 V environ
  000000000000000f W _start
  0000000000000042 W _start_c

and:

  $ printf "int main(void) { return (long)&strlen;}\n" | gcc -nostdlib -static -Isysroot/x86/include -include nolibc.h -Os -Wl,--gc-sections -xc -
  $ nm --size a.out 
  0000000000000004 V errno
  0000000000000006 T main
  0000000000000008 V _auxv
  0000000000000008 V environ
  000000000000000e t strlen
  000000000000000f W _start
  0000000000000042 W _start_c

> > As a side note, it seems strlcat()/strlcpy() fail to set the terminating null byte on some cases,

Indeed I've just checked and you're right, that defeats their purpose!

> > and the return code is not always the same as when using libbsd. It seems to be only on "error"
> > cases, and not sure if it's worth fixing all/some of those cases.

OK.

> > Let me know if you think it is worth adding some _simple_ patches (I don't think it is worth fixing
> > all the cases, the code is to fix all of the cases is probably not nice and not worth it).
> 
> Souns reasonable to me to fix the return values.
> And get some tests for it.

Seconded!

Thanks!
Willy