Unset D_FORTIFY_SOURCE and remove unneeded copied functions

[mjsir911]

Commit 4e2ab71 reverts micropython/micropython@5cf71b5 which solved micropython#6046, which I'm seeing again on my gentoo system with D_FORTIFY_HARDENED enabled by default.

Unfortunately we can't just revert the revertion because circuitpython enforces prototypes for defined functions, which is why 4e2ab71 was implemented initially. Micropython doesn't suffer from this issue.

The implemented fix is to just circumvent D_FORTIFY_SOURCE, a nice side-effect is we can remove the re-implemented functions that were added on the initial string.h removal.

[dhalbert]

Could you give more background on this? Which toolchain are you compiling with? We don't see this issue ourselves. Is it specific on gentoo? Is it is just when you compile ports/unix? If it's across all ports that use a cross-compiler you supply, if you used the ARM gcc 13.2 download, would you see this problem?

[mjsir911]

Could you give more background on this? Which toolchain are you compiling with?

I'm using gentoo's crossdev arm-none-eabi-gcc:

$ arm-none-eabi-gcc -v
Using built-in specs.
COLLECT_GCC=arm-none-eabi-gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/arm-none-eabi/12/lto-wrapper
Target: arm-none-eabi
Configured with: /var/tmp/portage/cross-arm-none-eabi/gcc-12.3.1_p20230526/work/gcc-12-20230526/configure --host=x86_64-pc-linux-gnu --target=arm-none-eabi --build=x86_64-pc-linux-gnu --prefix=/usr --bindir=/usr/x86_64-pc-linux-gnu/arm-none-eabi/gcc-bin/12 --includedir=/usr/lib/gcc/arm-none-eabi/12/include --datadir=/usr/share/gcc-data/arm-none-eabi/12 --mandir=/usr/share/gcc-data/arm-none-eabi/12/man --infodir=/usr/share/gcc-data/arm-none-eabi/12/info --with-gxx-include-dir=/usr/lib/gcc/arm-none-eabi/12/include/g++-v12 --with-python-dir=/share/gcc-data/arm-none-eabi/12/python --enable-languages=c --enable-obsolete --enable-secureplt --disable-werror --with-system-zlib --enable-nls --without-included-gettext --disable-libunwind-exceptions --enable-checking=release --with-bugurl=https://bugs.gentoo.org/ --with-pkgversion='Gentoo 12.3.1_p20230526 p2' --with-gcc-major-version-only --enable-lto --disable-libstdcxx-pch --enable-poison-system-directories --disable-libstdcxx-time --with-sysroot=/usr/arm-none-eabi --disable-gcov --disable-bootstrap --with-newlib --enable-multilib --disable-fixed-point --with-multilib-list=aprofile,rmprofile --disable-libgomp --disable-libssp --disable-libada --disable-cet --disable-systemtap --disable-valgrind-annotations --disable-vtable-verify --disable-libvtv --with-zstd --without-isl --disable-libsanitizer --disable-default-pie --enable-default-ssp --with-build-config=bootstrap-lto
Thread model: single
Supported LTO compression algorithms: zlib zstd
gcc version 12.3.1 20230526 (Gentoo 12.3.1_p20230526 p2) 

We don't see this issue ourselves.

This can be reproduced by defining the FORTIFY_SOURCE macro in the build process.

Is it specific on gentoo?

It happens on any toolchain built with source hardening.

Is it is just when you compile ports/unix? If it's across all ports that use a cross-compiler you supply,

I think this file is the only one affected.

If you used the ARM gcc 13.2 download, would you see this problem?

No, although if you follow the linked commits & issues you'll see that a function in this file was created to support source hardening and then was undone in 4e2ab71 so the defined function isn't necessary anymore. ensuring that source hardening is turned off circumvents this issue & workarounds (that don't work anymore)

See micropython#6046

$ make BOARD=adafruit_feather_rp2040 V=4

GEN build-adafruit_feather_rp2040/genhdr/mpversion.h
QSTR not updated
Module registrations not updated
Root pointer registrations not updated
In file included from /usr/arm-none-eabi/include/ssp/strings.h:34,
                 from /usr/arm-none-eabi/include/strings.h:77,
                 from /usr/arm-none-eabi/include/string.h:24,
                 from ../../shared/libc/string0.c:29:
../../shared/libc/string0.c:40:7: error: expected declaration specifiers or ‘...’ before numeric constant
   40 | void *memcpy(void *dst, const void *src, size_t n) {
      |       ^~~~~~
In file included from /usr/arm-none-eabi/include/string.h:180:
../../shared/libc/string0.c:40:7: error: expected ‘)’ before ‘!=’ token
   40 | void *memcpy(void *dst, const void *src, size_t n) {
      |       ^~~~~~
../../shared/libc/string0.c:40:7: error: expected ‘)’ before ‘?’ token
   40 | void *memcpy(void *dst, const void *src, size_t n) {
      |       ^~~~~~
../../shared/libc/string0.c:87:7: error: expected declaration specifiers or ‘...’ before numeric constant
   87 | void *memmove(void *dest, const void *src, size_t n) {
      |       ^~~~~~~
../../shared/libc/string0.c:87:7: error: expected ‘)’ before ‘!=’ token
   87 | void *memmove(void *dest, const void *src, size_t n) {
      |       ^~~~~~~
../../shared/libc/string0.c:87:7: error: expected ‘)’ before ‘?’ token
   87 | void *memmove(void *dest, const void *src, size_t n) {
      |       ^~~~~~~
../../shared/libc/string0.c:102:7: error: expected declaration specifiers or ‘...’ before numeric constant
  102 | void *memset(void *s, int c, size_t n) {
      |       ^~~~~~
../../shared/libc/string0.c:102:7: error: expected ‘)’ before ‘!=’ token
  102 | void *memset(void *s, int c, size_t n) {
      |       ^~~~~~
../../shared/libc/string0.c:102:7: error: expected ‘)’ before ‘?’ token
  102 | void *memset(void *s, int c, size_t n) {
      |       ^~~~~~
../../shared/libc/string0.c:188:7: error: expected declaration specifiers or ‘...’ before numeric constant
  188 | char *strcpy(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:188:7: error: expected ‘)’ before ‘!=’ token
  188 | char *strcpy(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:188:7: error: expected ‘)’ before ‘?’ token
  188 | char *strcpy(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:199:7: error: expected declaration specifiers or ‘...’ before numeric constant
  199 | char *strncpy(char *s1, const char *s2, size_t n) {
      |       ^~~~~~~
../../shared/libc/string0.c:199:7: error: expected ‘)’ before ‘!=’ token
  199 | char *strncpy(char *s1, const char *s2, size_t n) {
      |       ^~~~~~~
../../shared/libc/string0.c:199:7: error: expected ‘)’ before ‘?’ token
  199 | char *strncpy(char *s1, const char *s2, size_t n) {
      |       ^~~~~~~
../../shared/libc/string0.c:217:7: error: expected declaration specifiers or ‘...’ before numeric constant
  217 | char *stpcpy(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:217:7: error: expected ‘)’ before ‘!=’ token
  217 | char *stpcpy(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:217:7: error: expected ‘)’ before ‘?’ token
  217 | char *stpcpy(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:225:7: error: expected declaration specifiers or ‘...’ before numeric constant
  225 | char *strcat(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:225:7: error: expected ‘)’ before ‘!=’ token
  225 | char *strcat(char *dest, const char *src) {
      |       ^~~~~~
../../shared/libc/string0.c:225:7: error: expected ‘)’ before ‘?’ token
  225 | char *strcat(char *dest, const char *src) {
      |       ^~~~~~
make: *** [../../py/mkrules.mk:85: build-adafruit_feather_rp2040/shared/libc/string0.o] Error 1

[dhalbert]

The reason I'm asking is that in the past few merges from MicroPython sources, we've trying to reduce the differences between the files we share with MicroPython. So this change is an increase in skew. Could you fix this in both MicroPython and CircuitPython by guarding the definition of __memcpy_chk() with an #ifdef _FORTIFY_SOURCE instead of just removing it? If that is possible,could you also PR it upstream?

We are also labeling differences from upstream with // CIRCUITPY-CHANGE and a short explanation if it's not obvious. This makes it a lot easier to understand what is going one when we see a merge conflict, especially since we see the same conflicts over and over each time we merge from upstream.

[mjsir911]

The reason I'm asking is that in the past few merges from MicroPython sources, we've trying to reduce the differences between the files we share with MicroPython.

Okay, that makes a lot of sense.

Would a better approach then be to provide a string.h file to pull in locally with the prototype signatures required to define functions & then remove the string.h import? This will bring this file closer to micropython. The differentiating factor here is that circuitpython is requiring a prototype while I don't think micropython is?

[dhalbert]

For your particular case, can you leave our prototype declaration in and conditionalize the __memcpy_chk() definition and/or declaration as necessary? That would preserve the upstream definition but remove it for your compiles (if I understand right, that is the issue). This file already has changes from upstream.

Commit 4e2ab71 reverts micropython@5cf71b5 which solved micropython#6046,

I am little confused by this, because it don't see 4e2ab71 as a full revert, but just adding the declaration and changing the include. But I may be misunderstanding something.

In other words, can you keep the upstream declaration, and add/conditionalize whatever is necessary to make both our regular builds and your builds work?

[mjsir911]

I am little confused by this, because it don't see 4e2ab71 as a full revert, but just adding the declaration and changing the include. But I may be misunderstanding something.

Sorry for the confusion, thanks for bearing with me.

The issue I'm seeing was introduced with the string.h include which has macros that break compilation when FORTIFY_SOURCE is defined. micropython avoids this by not pulling in string.h entirely, which warranted adding the __memcpy_chk definition.

For your particular case, can you leave our prototype declaration in and conditionalize the __memcpy_chk() definition and/or declaration as necessary? That would preserve the upstream definition but remove it for your compiles (if I understand right, that is the issue). This file already has changes from upstream.

In other words, can you keep the upstream declaration, and add/conditionalize whatever is necessary to make both our regular builds and your builds work?

I can re-add the __memcpy_chk definition, builds still work with it, it was just removed since it was unnecessary following the re-inclusion of string.h, but I understand if y'all are optimizing for upstream-parity then keeping it in makes sense.

I'll say again though, if your goal is to maintain parity than the ideal way forward would probably be removing the string.h include & adding function prototypes to all defined functions in this file.

[jepler]

another possible alternative (only compile-tested, for adafruit feather rp2040, with -D_FORTIFY_SOURCE=1 manually added to CFLAGS)

commit 1650ce6d42e3106e2e60d0e2984dfcd0829d8162
Author: Jeff Epler <jepler@gmail.com>
Date:   Sat Nov 4 21:07:21 2023 +0100

    string0: Undo any definition of these to macros
    
    .. which can occur in <string.h>, particularly when -D_FORTIFY_SOURCE,
    which we otherwise rely on to provide a prototype.

diff --git a/shared/libc/string0.c b/shared/libc/string0.c
index b0a2620300..9e97002c29 100644
--- a/shared/libc/string0.c
+++ b/shared/libc/string0.c
@@ -28,6 +28,14 @@
 #include <stddef.h>
 #include <string.h>
 
+#undef memcpy
+#undef memmove
+#undef memset
+#undef stpcpy
+#undef strcpy
+#undef strcat
+#undef strncpy
+
 #include "py/mpconfig.h"
 
 #ifndef likely

[jepler]

yet another possibilty

diff --git a/py/circuitpy_defns.mk b/py/circuitpy_defns.mk
index 5f0af02a6e..701156cd1a 100644
--- a/py/circuitpy_defns.mk
+++ b/py/circuitpy_defns.mk
@@ -52,7 +52,8 @@ BASE_CFLAGS = \
 	-DCIRCUITPY_CANARY_WORD=0xADAF00 \
 	-DCIRCUITPY_SAFE_RESTART_WORD=0xDEADBEEF \
 	-DCIRCUITPY_BOARD_ID="\"$(BOARD)\"" \
-	--param max-inline-insns-single=500
+	--param max-inline-insns-single=500 \
+	-U_FORTIFY_SOURCE
 
 #        Use these flags to debug build times and header includes.
 #        -ftime-report

to disable _FORTIFY_SOURCE everywhere; this might get rid of the need to have the "chk" version of memcpy, too.

[mjsir911]

Yeah @jepler, disabling that flag everywhere might be preferred even if the issue has only exposed itself to me for this file (there could definitely be others, though)

both those solutions seem to work for me, thanks for acknowledging the intention behind the chk removal although it sounds like maintaining upstream parity is preferred

[jepler]

I'm closing this since we took one of the alternative solutions in #8663 -- please feel free to continue the conversation if this is still not resolved for you.