[PATCH 7/7] binman: Refuse to replace sections for now
Alper Nebi Yasak
alpernebiyasak at gmail.com
Sat Apr 23 17:46:57 CEST 2022
On 20/04/2022 00:54, Simon Glass wrote:
> On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>>
>> Binman interfaces allow attempts to replace any entry in the image with
>> arbitrary data. When trying to replace sections, the changes in the
>> section entry's data are not propagated to its child entries. This,
>> combined with how sections rebuild their contents from its children,
>> eventually causes the replaced contents to be silently overwritten by
>> rebuilt contents equivalent to the original data.
>>
>> Add a simple test for replacing a section that is currently failing due
>> to this behaviour, and mark it as an expected failure. Also, raise an
>> error when replacing a section instead of silently pretending it was
>> replaced.
>>
>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
>> ---
>>
>> tools/binman/etype/section.py | 3 +++
>> tools/binman/ftest.py | 9 ++++++++
>> .../test/234_replace_section_simple.dts | 23 +++++++++++++++++++
>> 3 files changed, 35 insertions(+)
>> create mode 100644 tools/binman/test/234_replace_section_simple.dts
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Is it not better to assertRaises() and check that the error message is
> as expected?
IMO, that would imply the tested 'binman replace' call should raise an
error instead of replacing the data. The test should work as-is, and
when section replacement is fixed we can just remove the expectedFailure
line to be strict about it.
>
>>
>> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
>> index ccac658c1831..bd67238b9199 100644
>> --- a/tools/binman/etype/section.py
>> +++ b/tools/binman/etype/section.py
>> @@ -788,6 +788,9 @@ def ReadChildData(self, child, decomp=True, alt_format=None):
>> data = new_data
>> return data
>>
>> + def WriteData(self, data, decomp=True):
>> + self.Raise("Replacing sections is not implemented yet")
>> +
>> def WriteChildData(self, child):
>> return True
>>
>> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
>> index 43bec4a88841..058651cc18a0 100644
>> --- a/tools/binman/ftest.py
>> +++ b/tools/binman/ftest.py
>> @@ -5641,6 +5641,15 @@ def testReplaceFitSubentryLeafSmallerSize(self):
>> self.assertIsNotNone(path)
>> self.assertEqual(expected_fdtmap, fdtmap)
>>
>> + @unittest.expectedFailure
>> + def testReplaceSectionSimple(self):
>> + """Test replacing a simple section with arbitrary data"""
>> + new_data = b'w' * len(COMPRESS_DATA + U_BOOT_DATA)
>> + data, expected_fdtmap, _ = self._RunReplaceCmd(
>> + 'section', new_data,
>> + dts='234_replace_section_simple.dts')
>> + self.assertEqual(new_data, data)
>> +
>>
>> if __name__ == "__main__":
>> unittest.main()
>> diff --git a/tools/binman/test/234_replace_section_simple.dts b/tools/binman/test/234_replace_section_simple.dts
>> new file mode 100644
>> index 000000000000..c9d5c3285615
>> --- /dev/null
>> +++ b/tools/binman/test/234_replace_section_simple.dts
>> @@ -0,0 +1,23 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +/ {
>> + binman {
>> + allow-repack;
>> +
>> + u-boot-dtb {
>> + };
>> +
>> + section {
>> + blob {
>> + filename = "compress";
>> + };
>> +
>> + u-boot {
>> + };
>> + };
>> +
>> + fdtmap {
>> + };
>> + };
>> +};
>> --
>> 2.35.1
>>
More information about the U-Boot
mailing list