On Mon, Mar 13, 2023 at 04:08:10AM +0100, Willy Tarreau wrote: > On Sun, Mar 12, 2023 at 11:12:50PM +0000, Thomas Weißschuh wrote: > > FYI there is also another patch to make nolibc-test buildable with > > compilers that enable -fstack-protector by default. > > Maybe this can be picked up until the proper stack-protector support is > > hashed out. > > Maybe even for 6.3: > > > > https://lore.kernel.org/lkml/20230221-nolibc-no-stack-protector-v1-1-4e6a42f969e2@xxxxxxxxxxxxxx/ > > Ah thanks, it seems I indeed missed it. It looks good, I'll take it. Do you have a tree with this published? So I can make sure the next revision of this patchset does not lead to conflicts. > > > > +int run_stackprotector(int min, int max) > > > > +{ > > > > + int llen = 0; > > > > + > > > > + llen += printf("0 "); > > > > + > > > > +#if !defined(NOLIBC_STACKPROTECTOR) > > > > + llen += printf("stack smashing detection not supported"); > > > > + pad_spc(llen, 64, "[SKIPPED]\n"); > > > > + return 0; > > > > +#endif > > > > > > Shouldn't the whole function be enclosed instead ? I know it's more of > > > a matter of taste, but avoiding to build and link it for archs that > > > will not use it may be better. > > > > The goal was to print a [SKIPPED] message if it's not supported. > > Ah indeed makes sense. > > > The overhead of doing this should be neglectable. > > It was not the overhead (that's only a regtest program after all), I > was more thinking about the difficulty to maintain this function over > time for other archs if it starts to rely on optional support. But for > now it's not a problem, it it would ever become one we could simply > change that to have a function just print SKIPPED. So I'm fine with > your option. > > > > > @@ -719,8 +784,11 @@ int prepare(void) > > > > /* This is the definition of known test names, with their functions */ > > > > static const struct test test_names[] = { > > > > /* add new tests here */ > > > > - { .name = "syscall", .func = run_syscall }, > > > > - { .name = "stdlib", .func = run_stdlib }, > > > > + { .name = "syscall", .func = run_syscall }, > > > > + { .name = "stdlib", .func = run_stdlib }, > > > > + { .name = "stackprotector", .func = run_stackprotector, }, > > > > + { .name = "_smash_stack", .func = run_smash_stack, > > > > > > I think it would be better to keep the number of categories low > > > and probably you should add just one called "protection" or so, > > > and implement your various tests in it as is done for other > > > categories. The goal is to help developers quickly spot and select > > > the few activities they're interested in at a given moment. > > > > I'm not sure how this would be done. The goal here is that > > "stackprotector" is the user-visible category. It can be changed to > > "protection". > > "_smash_stack" however is just an entrypoint that is used by the forked > > process to call the crashing code. > > Ah I didn't realize that, I now understand how that can be useful, > indeed. Then maybe just rename your .skip_by_default field to .hidden > so that it becomes more generic (i.e. if one day we permit enumeration > we don't want such tests to be listed either), and assign the field on > the same line so that it's easily visible with a grep. Actually this works fine with a plain fork() and the exec() is not needed. So the dedicated entrypoint is not needed anymore. No idea what I tested before.
