search

Yubico / libfido2

Provides library functionality for FIDO2, including communication with a device over USB or NFC.
Other
590 stars 152 forks source link

build: unbreak builds with _FORTIFY_SOURCE=1 #708

Open ynezz opened 1 year ago

ynezz commented 1 year ago

Some downstream build systems allow passing custom _FORTIFY_SOURCE values and this results in conflicting compiler flags and build failures:

 <command-line>: error: "_FORTIFY_SOURCE" redefined

So lets fix it by undefining any previous _FORTIFY_SOURCE values and then forcing _FORTIFY_SOURCE=2.

ynezz commented 1 year ago

Example of such failure https://github.com/openwrt/packages/actions/runs/5410066043/jobs/9831013719

[1/65] Building C object src/CMakeFiles/fido2_shared.dir/aes256.c.o
FAILED: src/CMakeFiles/fido2_shared.dir/aes256.c.o 
/builder/staging_dir/toolchain-arm_cortex-a15+neon-vfpv4_gcc-12.3.0_musl_eabi/bin/arm-openwrt-linux-muslgnueabi-gcc -DHAVE_ASPRINTF -DHAVE_CBOR_H -DHAVE_CLOCK_GETTIME -DHAVE_DEV_URANDOM -DHAVE_ENDIAN_H -DHAVE_ERR_H -DHAVE_EXPLICIT_BZERO -DHAVE_GETLINE -DHAVE_GETOPT -DHAVE_GETPAGESIZE -DHAVE_GETRANDOM -DHAVE_OPENSSLV_H -DHAVE_POSIX_IOCTL -DHAVE_SIGNAL_H -DHAVE_STRLCAT -DHAVE_STRLCPY -DHAVE_STRSEP -DHAVE_SYSCONF -DHAVE_SYS_RANDOM_H -DHAVE_UNISTD_H -DOPENSSL_API_COMPAT=0x10100000L -DTLS=__thread -DUSE_NFC -D_FIDO_INTERNAL -D_FIDO_MAJOR=1 -D_FIDO_MINOR=12 -D_FIDO_PATCH=0 -Dfido2_shared_EXPORTS -I/builder/build_dir/target-arm_cortex-a15+neon-vfpv4_musl_eabi/libfido2-1.12.0/src -D_POSIX_C_SOURCE=200809L -D_BSD_SOURCE -D_GNU_SOURCE -D_DEFAULT_SOURCE -std=c99 -Os -pipe -fno-caller-saves -fno-plt -fhonour-copts -mfloat-abi=hard -ffile-prefix-map=/builder/build_dir/target-arm_cortex-a15+neon-vfpv4_musl_eabi/libfido2-1.12.0=libfido2-1.12.0 -Wformat -Werror=format-security -fstack-protector -D_FORTIFY_SOURCE=1 -Wl,-z,now -Wl,-z,relro -DNDEBUG -D_FORTIFY_SOURCE=2 -fPIC -Wall -Wextra -Werror -Wshadow -Wcast-qual -Wwrite-strings -Wmissing-prototypes -Wbad-function-cast -Wimplicit-fallthrough -pedantic -pedantic-errors -fstack-protector-all -Wno-unused-result -Wconversion -Wsign-conversion -Wframe-larger-than=2047 -MD -MT src/CMakeFiles/fido2_shared.dir/aes256.c.o -MF src/CMakeFiles/fido2_shared.dir/aes256.c.o.d -o src/CMakeFiles/fido2_shared.dir/aes256.c.o -c /builder/build_dir/target-arm_cortex-a15+neon-vfpv4_musl_eabi/libfido2-1.12.0/src/aes256.c
<command-line>: error: "_FORTIFY_SOURCE" redefined
<command-line>: note: this is the location of the previous definition
ninja: build stopped: subcommand failed.
LDVG commented 1 year ago

Hm, wouldn't it be preferred to honor the environment's wishes for a specific _FORTIFY_SOURCE level?

ynezz commented 1 year ago

Hm, wouldn't it be preferred to honor the environment's wishes for a specific _FORTIFY_SOURCE level?

I've looked at fadcc9847b13 introducing this option and have assumed, that its desired. Would for example _FORTIFY_SOURCE=3 work out of the box? Either way, I'm fine adjusting it as desired.

LDVG commented 1 year ago

I've looked at https://github.com/Yubico/libfido2/commit/fadcc9847b137b2ddff64e7853c1c83ec2152806 introducing this option and have assumed, that its desired.

It should be seen as our desired default for release builds at this time. But I'd argue that if -D_FORTIFY_SOURCE is set by the caller, we should assume that they know what they're doing respect its value.

Would for example _FORTIFY_SOURCE=3 work out of the box?

I've only tested it briefly; it certainly should work. If it doesn't, then that's probably a bug.

ynezz commented 1 year ago

we should assume that they know what they're doing respect its value.

Ok, I agree and I'll rework it.