Clarify -D_FORTIFY_SOURCE interaction with optimization levels
#620
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes the -D_FORTIFY_SOURCE table in the detailed section consistent with the recommended option table. Signed-off-by: Thomas Nyman <[email protected]>
This makes the -D_FORTIFY_SOURCE table in the detailed section consistent with how other options have their variants ordered. Signed-off-by: Thomas Nyman <[email protected]>
…RTIFY_SOURCE Signed-off-by: Thomas Nyman <[email protected]>
thomasnyman
changed the title
-D_FORTIFY_SOURCE interaction with optimization levels
siddhesh
reviewed
Sep 19, 2024
| #### Additional Considerations | ||
|
|
||
| - Applications that incorrectly use `malloc_usable_size`[^malloc_usable_size] to use the additional size reported by the function may abort at runtime. This is a bug in the application because the additional size reported by `malloc_usable_size` is not generally safe to dereference and is for diagnostic uses only. The correct fix for such issues is to avoid using `malloc_usable_size` as the glibc manual specifically states that it is for diagnostic purposes *only* [^malloc_usable_size]. On many Linux systems these incorrect uses can be detected by running `readelf -Ws <path>` on the ELF binaries and searching for `malloc_usable_size@GLIBC`[^kpyrd23]. If avoiding `malloc_usable_size` is not possible, one may call `realloc` to resize the block to its usable size and to benefit from `_FORTIFY_SOURCE=3`. | ||
| Internally `-D_FORTIFY_SOURCE` relies on the built-in functions for object size checking in GCC[^gcc-objectsizechecks] and Clang[^clang-evaluatingobjectsize], namely `__builtin_object_size` and `__builtin_dynamic_object_size`. These builtins provide conservative approximations of the object size and are sensitive to compiler optimizations. With optimization enabled they produce more accurate estimates, especially when a call to `__builtin_object_size` is in a different function from where its argument pointer is formed. In addition, GCC allows more information about subobject bounds to be determined with higher optimization levels. Hence we recommending enabling `-D_FORTIFY_SOURCE=3` with at least optimization level `-O2`. |
There was a problem hiding this comment.
I wouldn't call out the subobject bounds so prominently because in practice better optimization can also sometimes result in subobject information being lost. I think a more reliable claim would be that higher optimization allows more number of __builtin_object_size and __builtin_dynamic_object_size calls to succeed, not the accuracy of their size estimates.
| - Applications that incorrectly use `malloc_usable_size`[^malloc_usable_size] to use the additional size reported by the function may abort at runtime. This is a bug in the application because the additional size reported by `malloc_usable_size` is not generally safe to dereference and is for diagnostic uses only. The correct fix for such issues is to avoid using `malloc_usable_size` as the glibc manual specifically states that it is for diagnostic purposes *only* [^malloc_usable_size]. On many Linux systems these incorrect uses can be detected by running `readelf -Ws <path>` on the ELF binaries and searching for `malloc_usable_size@GLIBC`[^kpyrd23]. If avoiding `malloc_usable_size` is not possible, one may call `realloc` to resize the block to its usable size and to benefit from `_FORTIFY_SOURCE=3`. | ||
| Internally `-D_FORTIFY_SOURCE` relies on the built-in functions for object size checking in GCC[^gcc-objectsizechecks] and Clang[^clang-evaluatingobjectsize], namely `__builtin_object_size` and `__builtin_dynamic_object_size`. These builtins provide conservative approximations of the object size and are sensitive to compiler optimizations. With optimization enabled they produce more accurate estimates, especially when a call to `__builtin_object_size` is in a different function from where its argument pointer is formed. In addition, GCC allows more information about subobject bounds to be determined with higher optimization levels. Hence we recommending enabling `-D_FORTIFY_SOURCE=3` with at least optimization level `-O2`. | ||
|
|
||
| Applications that incorrectly use `malloc_usable_size`[^malloc_usable_size] to use the additional size reported by the function may abort at runtime. This is a bug in the application because the additional size reported by `malloc_usable_size` is not generally safe to dereference and is for diagnostic uses only. The correct fix for such issues is to avoid using `malloc_usable_size` as the glibc manual specifically states that it is for diagnostic purposes *only* [^malloc_usable_size]. On many Linux systems these incorrect uses can be detected by running `readelf -Ws <path>` on the ELF binaries and searching for `malloc_usable_size@GLIBC`[^kpyrd23]. If avoiding `malloc_usable_size` is not possible, one may call `realloc` to resize the block to its usable size and to benefit from `_FORTIFY_SOURCE=3`. |
There was a problem hiding this comment.
systemd for example uses a dummy realloc hack to fool the compiler into thinking that the larger block is available, but I don't know if it's a good idea to recommend such practice:
https://github.com/systemd/systemd/blob/main/src/basic/alloc-util.h#L192
Also, I had introduced a fast path in realloc in glibc-2.37 that makes such hacks unnecessary since the glibc realloc should return the same block immediately if the new size fits into it. That may or may not be true for alternate allocators though.
There was a problem hiding this comment.
If we're in the business of telling people what best practice is, we should indeed actively tell them not to do that at all. The only reason we haven't completely killed it in glibc is because we can't quite agree on how to move forward, but everyone accepts that using it like this is bad.
(I haven't brought it up again given that I haven't hit any other instances.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
-D_FORTIFY_SOURCEoptimization level requirement to the description in the detailed section table-D_FORTIFY_SOURCEvariants in the detailed section table to be consistent with the ordering of other options-D_FORTIFY_SOURCEperformance