buffer: add optional -fbounds-safety annotations for struct buffer - #1105
LaptopsPlural wants to merge 4 commits into
Conversation
Introduce inert OVPN_SIZED_BY*_ macros (OFF by default) and annotate the struct buffer data/capacity pair used on packet paths. Capacity-first assign already present in alloc paths (documented). Default builds unchanged; ENABLE_FBOUNDS_SAFETY / --enable-fbounds-safety opt-in for experimental Clang toolchains. Signed-off-by: Jeff <jeff@incrediblybased.co>
Align trailing comments and preprocessor macros with project .clang-format so the style CI check passes. Signed-off-by: Jeff <jeff@incrediblybased.co>
flichtenheld
left a comment
flichtenheld
left a comment
There was a problem hiding this comment.
Definitely looks interesting. The changes are overly verbose, see my comments. My understanding is that this is currently only available in Apple's clang fork?
|
|
||
| #else /* !OVPN_SUPPORT_FBOUNDS_SAFETY */ | ||
|
|
||
| #define OVPN_SIZED_BY(n) |
There was a problem hiding this comment.
Why use the additional indirection? Can't we just do
#define __sized_by(n)
| # macros in src/openvpn/buffer_bounds_safety.h are inert and the ABI/build is unchanged. | ||
| # When ON, requires a Clang that provides -fbounds-safety / <ptrcheck.h>. | ||
| option(ENABLE_FBOUNDS_SAFETY "Enable experimental Clang -fbounds-safety annotations (OFF by default)" OFF) | ||
| if(ENABLE_FBOUNDS_SAFETY) |
There was a problem hiding this comment.
We have no status message for other options, so this is not required
| * within the allocated memory. */ | ||
| int len; /**< Length in bytes of the actual content | ||
| * within the allocated memory. */ | ||
| /* Field order already has capacity before data; alloc / set |
There was a problem hiding this comment.
Comment is mostly redundant
| uint8_t *data; /**< Pointer to the allocated memory. */ | ||
| int capacity; /**< Size in bytes of memory allocated by | ||
| * \c malloc(). Capacity companion for | ||
| * \c data under optional -fbounds-safety. */ |
There was a problem hiding this comment.
comment change is redundant and not required
| { | ||
| buf_size_error(size); | ||
| } | ||
| /* Capacity before pointer so sized_by invariants hold under -fbounds-safety. */ |
There was a problem hiding this comment.
All these additional comments are not required
There was a problem hiding this comment.
Not sure this warrants its own file. Might be small enough to put into compat.h or syshead.h
Move inert __sized_by* stubs into compat.h, drop OVPN_* wrappers and buffer_bounds_safety.h, restore compact buffer comments, and remove ENABLE_FBOUNDS_SAFETY status chatter from CMake/configure. Signed-off-by: Jeff <jeff@incrediblybased.co>
|
Thanks @flichtenheld — good catches. Slimmed this per your review:
On Apple Clang: yes — |
2 participants
Summary
Secure-by-design memory-safety hardening. Annotates
struct buffer.datawith optional Clang-fbounds-safety/ sized-by macros tied tocapacity. Default builds unchanged (ENABLE_FBOUNDS_SAFETYOFF).Per CONTRIBUTING: GitHub PRs are for discussion; will also send
git format-patchto openvpn-devel / Gerrit for merge.Contributor: Jeff Bindel via
LaptopsPlural. Not a vulnerability PoC.Test plan