Skip to content

Commit

Permalink
Add FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS (#289)
Browse files Browse the repository at this point in the history
Work around issue where other builders do not align empty vectors correctly.
  • Loading branch information
bkietz authored Aug 19, 2024
1 parent 4b21380 commit fd3c4ae
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 27 deletions.
11 changes: 11 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ option (FLATCC_DEBUG_VERIFY
option (FLATCC_TRACE_VERIFY
"assert on verify failure in runtime lib" OFF)

# Some producers allow empty vectors to be misaligned.
# The following setting will cause the verifier to require the index 0
# position to be element aligned even if the vector is empty (otherwise that
# position is only required to be aligned to the preceding size field).
option (FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS
"verify includes full alignment check for empty vectors" OFF)

# Reflection is the compilers ability to generate binary schema output
# (.bfbs files). This requires using generated code from
# `reflection.fbs`. During development it may not be possible to
Expand Down Expand Up @@ -141,6 +148,10 @@ if (FLATCC_TRACE_VERIFY)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DFLATCC_TRACE_VERIFY=1")
endif()

if (FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DFLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS=1")
endif()


if (FLATCC_REFLECTION)
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DFLATCC_REFLECTION=1")
Expand Down
27 changes: 15 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1164,18 +1164,20 @@ sufficient, but for example standard 32-bit Windows only allocates on an
aligned fields.

NOTE: as of August 2024 it has been discovered that C++ writer code has been
aligning empty vectors to the size field only, even if elements where larger,
like the double type of size 8. This would cause the FlatCC verifier to
(correctly) reject these vectors because it would result in an invalid C pointer
type on some architectures. However, because this has been in effect for over
10 years, the consensus is to have verifiers tolerate this behaviour even if
C++ will eventually fix this issue. The FlatCC verifier will be updated to
accept such buffers by default with a compile time flag to enforce the strict
behaviour as well. In principle the misaligned vectors can lead potentially
lead to undefined behaviour in agressively optimized C compilers. As of now
it appears to be safe to read such buffers on common platforms and it is
preferably to avoid additional runtime reader overhead to deal with this.
For more, see [FlatCC #287], [Google Flatbuffers #8374].
aligning empty vectors to the size field only, even if elements require
greater alignment like the double type which requires 8. This would cause the
FlatCC verifier to (correctly) reject these vectors because it would result
in an invalid C pointer type on some architectures. However, because this has
been in effect for over 10 years, the consensus is to have verifiers tolerate
this behaviour even if C++ will eventually fix this issue. The FlatCC
verifier has been updated to accept such buffers by default with an optional
compile time flag to enforce the strict behaviour as well
(`FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS`). In principle the misaligned vectors
can lead potentially lead to undefined behaviour in agressively optimized C
compilers. As of now it appears to be safe to read such buffers on common
platforms and it is preferable to avoid additional runtime reader overhead to
deal with this. For more, see [FlatCC #287], [Google Flatbuffers #8374],
[FlatCC #289].


### Potential Name Conflicts
Expand Down Expand Up @@ -2718,3 +2720,4 @@ See [Benchmarks]
[WebKit Style]: https://webkit.org/code-style-guidelines/
[FlatCC #287]: https://github.com/dvidelabs/flatcc/issues/287
[Google Flatbuffers #8374]: https://github.com/google/flatbuffers/issues/8374
[FlatCC #289]: https://github.com/dvidelabs/flatcc/issues/289
9 changes: 9 additions & 0 deletions include/flatcc/flatcc_rtconfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ extern "C" {
#define FLATCC_TRACE_VERIFY 0
#endif

/*
* Some producers allow empty vectors to be misaligned.
* The following setting will cause the verifier to require the index 0
* position to be element aligned even if the vector is empty (otherwise that
* position is only required to be aligned to the preceding size field).
*/
#if !defined(FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS)
#define FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS 0
#endif

/*
* Limit recursion level for tables. Actual level may be deeper
Expand Down
23 changes: 8 additions & 15 deletions src/runtime/verifier.c
Original file line number Diff line number Diff line change
Expand Up @@ -135,20 +135,6 @@ static inline int check_header(uoffset_t end, uoffset_t base, uoffset_t offset)
return k > base && k + offset_size <= end && !(k & (offset_size - 1));
}

static inline int check_aligned_header(uoffset_t end, uoffset_t base, uoffset_t offset, uint16_t align)
{
uoffset_t k = base + offset;

if (uoffset_size <= voffset_size && k + offset_size < k) {
return 0;
}
/* Alignment refers to element 0 and header must also be aligned. */
align = align < uoffset_size ? uoffset_size : align;

/* Note to self: the builder can also use the mask OR trick to propagate `min_align`. */
return k > base && k + offset_size <= end && !((k + offset_size) & ((offset_size - 1) | (align - 1u)));
}

static inline int verify_struct(uoffset_t end, uoffset_t base, uoffset_t offset, uoffset_t size, uint16_t align)
{
/* Structs can have zero size so `end` is a valid value. */
Expand Down Expand Up @@ -278,10 +264,17 @@ static inline int verify_vector(const void *buf, uoffset_t end, uoffset_t base,
{
uoffset_t n;

verify(check_aligned_header(end, base, offset, align), flatcc_verify_error_vector_header_out_of_range_or_unaligned);
verify(check_header(end, base, offset), flatcc_verify_error_vector_header_out_of_range_or_unaligned);
base += offset;

n = read_uoffset(buf, base);
base += offset_size;

#if !FLATCC_ENFORCE_ALIGNED_EMPTY_VECTORS
/* This is due to incorrect buffers from other builders than cannot easily be ignored. */
align = n == 0 ? uoffset_size : align;
#endif
verify(!(base & ((align - 1u) | (uoffset_size - 1u))), flatcc_verify_error_vector_header_out_of_range_or_unaligned);
/* `n * elem_size` can overflow uncontrollably otherwise. */
verify(n <= max_count, flatcc_verify_error_vector_count_exceeds_representable_vector_size);
verify(end - base >= n * elem_size, flatcc_verify_error_vector_out_of_range);
Expand Down

0 comments on commit fd3c4ae

Please sign in to comment.