fix: write NUL terminator for MAM coherency signature - #601
Conversation
Zero the buffer so every byte written to the MAM is defined. In particular the "LTFS\0" signature below is copied with arch_strncpy(...,"LTFS",5,4), which on Linux/Mac maps to strncpy(dst,"LTFS",4) and does NOT write the 5th (NUL) byte. tape_get_cart_coherency() validates the signature with strncmp(...,"LTFS",5), so leaving that byte as uninitialised stack made coherency validation depend on garbage: when non-zero, every mount fails with LTFS12062W and falls back to a full medium consistency check.
No change to the on-medium bytes; this hardens how the "LTFS" volume coherency signature is written and checked: - Write the signature with memcpy of the full "LTFS\0" (5 bytes) instead of arch_strncpy(..., 5, 4), which copied only 4 bytes and relied on the preceding memset to supply the NUL terminator. The NUL is now explicit. - Move the literal into TC_MAM_COHERENCY_SIGNATURE (tape_ops.h) so the writer and reader share one definition of the magic value and its compared length. - Add _Static_asserts that the page buffer is large enough to hold every fixed field offset that is written, and that the signature is 4 chars plus a NUL, catching regressions at compile time.
|
Added some hardening on this part using C11 |
Apply the same compile-time bounds check used for the coherency page to the other two fixed-offset MAM buffers in tape.c: - tape_get_volume_change_reference reads a 32-bit VCR at offset 5, so assert the page holds at least 5 + sizeof(uint32_t) bytes. - tape_get_cart_volume_lock_status reads the status byte at offset TC_MAM_PAGE_HEADER_SIZE, so assert the page extends past the header. Both buffer sizes derive from page-size constants in tape_ops.h; the asserts fail the build if those constants are ever reduced below what the fixed-offset reads require, instead of silently reading past the end of the stack buffer.
There was a problem hiding this comment.
Hello @hugo-hur ,
Thanks for this catch too! Your help is greatly appreciated, the changes overall look good, but I am not convinced to add the static assertions.
I understand the objective to prevent future breaking changes regarding the values that define the buffers size, but reading them in code seems kinda redundant since the buffers are being declared just above.
Thanks for the feedback. I will remove those static asserts if other reviewers also agree that this mam handling is better without. |
XV02
left a comment
XV02
left a comment
There was a problem hiding this comment.
Thanks for solving this warning, just a few correctness comments :DD
The _Static_asserts added while hardening the MAM coherency handling restated the buffer declarations a few lines above and reintroduced magic numbers (74, 5) that duplicate the fixed offsets. They add no real protection -- a page-layout change would require rewriting the offset writes regardless -- so drop them per review feedback. The NUL-terminator fix, the explicit "LTFS" signature memcpy, and the TC_MAM_COHERENCY_SIGNATURE definition are retained.
Replace the hardcoded byte offsets in the volume coherency read/write paths (and the shared MAM attribute header offsets they use) with named TC_MAM_* defines in tape_ops.h, per review feedback about magic numbers. The coherency writer and reader, the VCR reader, and the lock-status reader now index their buffers by name, and the UUID copy uses sizeof(coh->uuid). Pure refactor -- the on-medium bytes are unchanged; the one non-trivial rewrite (the application-client-specific-information length, previously two byte writes, now a single ltfs_u16tobe) was verified byte-identical.
XV02
left a comment
XV02
left a comment
There was a problem hiding this comment.
Thanks for removing the magic numbers and properly naming the offsets, just some correctness comments :DD
Writes to the coh_data are now fully deteministic using the memcpy so no need to first zero the buffer. The memset's original job, supplying the "LTFS" signature's NUL terminator, is now done by the explicit memcpy.
3 participants
Zero the buffer so every byte written to the MAM is defined. In particular the "LTFS\0" signature below is copied with arch_strncpy(...,"LTFS",5,4), which on Linux/Mac maps to strncpy(dst,"LTFS",4) and does NOT write the 5th (NUL) byte. tape_get_cart_coherency() validates the signature with strncmp(...,"LTFS",5), so leaving that byte as uninitialised stack made coherency validation depend on garbage: when non-zero, every mount fails with LTFS12062W and falls back to a full medium consistency check.
Summary of changes
This pull request includes following changes or fixes.
Description
I saw random warnings on fallback to media consistency check. When digging into this it seems there is uninitialized memory issue which is simply fixed by memsetting the allocated mam coherency data array. After adding the null initialization the issue disappears.
Type of change
Checklist: