Merge request https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277 was reviewed by Gedare Bloom
-- Gedare Bloom started a new discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117280 > + * > + * @note: This function is invoked if a stack overflow is detected > + * if GCC flag -fstack-protection is enabled. Typo: `-fstack-protector` -- Gedare Bloom started a new discussion on cpukit/include/rtems/stackchk.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117281 > + * if GCC flag -fstack-protection is enabled. > + */ > +void __stack_chk_fail(void); Is there a way to check if the `-fstack-protector` has been enabled? It might be worth only defining this support if it's been actually used. -- Gedare Bloom started a new discussion on cpukit/include/rtems/confdefs/extensions.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117282 > > +#ifndef CONFIGURE_STACK_CHECKER_GUARD_VALUE > + #define CONFIGURE_STACK_CHECKER_GUARD_VALUE 0xDEADBEEF Doesn't `-fstack-protector` create it's own canaries? Why do we need this? -- Gedare Bloom started a new discussion on cpukit/libmisc/shell/main_stackuse.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117283 > * On-Line Applications Research Corporation (OAR). > - * > + * Don't change random white space. -- Gedare Bloom commented on a discussion on cpukit/libmisc/shell/main_stackuse.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117284 > #include <rtems/shell.h> > #include <rtems/score/threadimpl.h> > +#include <rtems/score/basedefs.h> These attributes aren't used in this file. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/stackchk05.scn: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117285 > +*** TEST STACKCHK05 *** This isn't the test output. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk06/config.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117286 > + > +#define CONFIGURE_INIT > +#include "../stackchk05/system.h" This isn't exactly great. It creates a dependency between tests. As I wrote above you probably need to change some of the shared code anyway. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk06/config.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117287 > + > +/* configuration information */ > +#define CONFIGURE_STACK_CHECKER_ENABLED Should the `-fstack-protector` also work without this CONFIGURE enabled? If so, there should be a test for it. With this CONFIGURE enabled, you have to check the `printk` output to be sure that `__stack_chk_fail` was called. This does not seem to be ideal. I think you should rewrite the fatal extension handler in this test to be a failure path for the test, and put the `TEST_END` directly here in `__stack_chk_fail`. You can just not provide a fatal extension handler in this test. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117288 > + strcpy(buffer_short, long_string); > + > + printk("Stack Overflow invoked\n"); I would prefer to have nothing printed for a successful test. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/init.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117289 > + invoke_stack_overflows(); > + > + rtems_task_exit(); Add a comment that this is a failure path for this test. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk06/config.c: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117290 > +{ > + // Custom handling for stack overflow > + printk("Custom stack overflow handler invoked\n"); If you modify the test to remove the fatal extension handler, then you don't need to print here. -- Gedare Bloom started a new discussion on testsuites/libtests/stackchk05/system.h: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277#note_117291 > +#define CONFIGURE_APPLICATION_NEEDS_CLOCK_DRIVER > + > +#define TASK_STACK_SIZE (RTEMS_MINIMUM_STACK_SIZE*3) This test doesn't need all these extra configuration options. That said, I'm surprised this test works considering the additional stack size that is allocated. The stack checker should only detect a blown stack that goes beyond the task's stack size. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/277 You're receiving this email because of your account on gitlab.rtems.org.
_______________________________________________ bugs mailing list bugs@rtems.org http://lists.rtems.org/mailman/listinfo/bugs
