Re: [PATCH 7/7] binman: Refuse to replace sections for now

[Simon Glass <sjg@chromium.org>]

On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak@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@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@chromium.org>

Is it not better to assertRaises() and check that the error message is
as expected?


>
> 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
>