TDBStore: pad program units when writing record_header_t; ensure work buffer is large enough #13996
Conversation
This file contains 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
Previously, when writing a record header into a TDBStore, we took the pointer to the record_header_t variable as the input, but used a program-aligned chunk size which is larger. As a result, garbage from the stack memory gets written. This commit fixes that by buffering the record header. Co-authored-by: Seppo Takalo <seppo.takalo@arm.com>
Previously, we always set the work buffer to 64 bytes, without checking it was no less the actual program size. But we can't simply switch to use the program size for the work buffer, because if it's too small (e.g. 1 byte in some cases), we will not be able to read the status header (24 bytes), and small buffers means more underlying write operations and lower efficiency. This PR changes the work buffer size to be the program size, or 64 bytes as an absolute minimum like before.
Co-authored-by: Seppo Takalo <seppo.takalo@arm.com>
ciarmcom
added
the
release-type: patch
|
@LDong-Arm, thank you for your changes. |
evedon
approved these changes
Dec 7, 2020
| @@ -847,7 +847,10 @@ int TDBStore::copy_record(uint8_t from_area, uint32_t from_offset, uint32_t to_o | |||
| } | |||
|
|
|||
| chunk_size = align_up(sizeof(record_header_t), _prog_size); | |||
| ret = write_area(1 - from_area, to_offset, chunk_size, &header); | |||
| // The record header takes up whole program units | |||
| memset(_work_buf, 0, chunk_size); | |||
There was a problem hiding this comment.
I thought that a check thatchunk_size is not greater than _work_buf_size was missing but I see that this is done in TDBStore::init()
|
Either this or #13908 need to be rebased, when one of them is merged |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
0xc0170
approved these changes
Dec 9, 2020
mbedmain
added
release-version: 6.6.0
Release-pending
and removed
release-type: patch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary of changes
(Continuation of #13731)
When copying a TDB record header, we program whole program unit(s) as the underlying BlockDevice requires. Previously, we directly passed the address of the variable
record_header_t headeras the input buffer, but if the program size is larger thansizeof(record_header_t)(24 bytes), data beyond the variable (on the stack) gets programmed into the BlockDevice as a result. To fix this, this PR copies the record header variable into the zero-padded work buffer first, then writes the work buffer to the storage.Additionally, this PR ensures the work buffer is at least the program size (this check was missing previously) and also no less than 64 byte. Note: To accommodate the
record_header_tthe minimum size is 24, but 64 bytes is what we had before and a power of 2.#13731 also has small improvements to the TDBStoreModuleTest, which we carry forward to this PR.
Please see individual commits, as several changes are involved.
Impact of changes
No impact in terms of public API or functionality.
Migration actions required
None.
Documentation
None.
Pull request type
Test results
Reviewers
@ARMmbed/mbed-os-core