[PATCH 0/7] binman: Fix replacing FIT subentries

[PATCH 0/7] binman: Fix replacing FIT subentries

[Alper Nebi Yasak]

Converting the binman FIT entry type to a section ended up breaking the
ability to replace these in images. The conversion to section partially
enables extracting and replacing the subentries, but that also doesn't
work as intended.

This series gets binman to a point where it can extract FIT subentries
and replace their non-section leaf entries correctly. Replacing sections
of any kind doesn't really work right now, so the final patch disables
that by raising an error.

Initially, I had managed to replace sections by propagating changes in
ProcessContentsUpdate() to the child entries, but writing arbitrary data
into entries representing data in a certain format quickly gets into
weird (maybe even undecidable?) edge cases.

Just recently I had a better idea. Instead of writing incompatible data
into a section, we can replace the section entry/node with for example a
blob-ext entry/node for the input file. I've got this working as a proof
of concept, but I need to experiment more to see what works best.


Alper Nebi Yasak (7):
  binman: Fix unique names having '/.' for images read from files
  binman: Collect bintools for images when replacing entries
  binman: Don't reset offset/size if image doesn't allow repacking
  binman: Remove '/images/' fragment from FIT subentry paths
  binman: Create FIT subentries in the FIT section, not its parent
  binman: Test replacing non-section entries in FIT subsections
  binman: Refuse to replace sections for now

 tools/binman/control.py                       |   3 +-
 tools/binman/entry.py                         |   2 +-
 tools/binman/etype/_testing.py                |  36 ++++
 tools/binman/etype/fit.py                     |  15 +-
 tools/binman/etype/section.py                 |   3 +
 tools/binman/ftest.py                         | 179 ++++++++++++++++++
 tools/binman/test/230_unique_names.dts        |  34 ++++
 tools/binman/test/231_unique_names_multi.dts  |  38 ++++
 .../binman/test/232_replace_with_bintool.dts  |  39 ++++
 tools/binman/test/233_fit_extract_replace.dts |  74 ++++++++
 .../test/234_replace_section_simple.dts       |  23 +++
 11 files changed, 438 insertions(+), 8 deletions(-)
 create mode 100644 tools/binman/test/230_unique_names.dts
 create mode 100644 tools/binman/test/231_unique_names_multi.dts
 create mode 100644 tools/binman/test/232_replace_with_bintool.dts
 create mode 100644 tools/binman/test/233_fit_extract_replace.dts
 create mode 100644 tools/binman/test/234_replace_section_simple.dts

-- 
2.35.1

[PATCH 1/7] binman: Fix unique names having '/.' for images read from files

[Alper Nebi Yasak]

Binman can embed a copy of the image description into the images it
builds as a fdtmap entry, but it omits the /binman/<image-name> prefix
from the node paths while doing so. When reading an already-built image
file, entries are reconstructed using this fdtmap and their associated
nodes still lack that prefix.

Some entries like fit and vblock create intermediate files whose names
are based on an entry unique name. This name is constructed from their
node's path by concatenating the parents with dots up to the binman
node, e.g. /binman/image/foo/bar becomes 'image.foo.bar'.

However, we don't have this /binman/image prefix when replacing entries
in such an image. The /foo/bar entry we read when doing so erroneously
has the unique name of '/.foo.bar', causing permission errors when the
entry attempts to create files based on that.

Fix the unique-name generation by stopping at the '/' node like how it
stops at the binman node. As the unique names are used as filenames, add
tests that check if they're safe to use as filenames.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/entry.py                        |  2 +-
 tools/binman/ftest.py                        | 31 ++++++++++++++++
 tools/binman/test/230_unique_names.dts       | 34 ++++++++++++++++++
 tools/binman/test/231_unique_names_multi.dts | 38 ++++++++++++++++++++
 4 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/230_unique_names.dts
 create mode 100644 tools/binman/test/231_unique_names_multi.dts

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 18a7a3510548..a07a5888643a 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -775,7 +775,7 @@ def GetUniqueName(self):
         node = self._node
         while node.parent:
             node = node.parent
-            if node.name == 'binman':
+            if node.name in ('binman', '/'):
                 break
             name = '%s.%s' % (node.name, name)
         return name
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 876953f11324..e6f0159a229f 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5471,6 +5471,37 @@ def testFitSplitElfFaked(self):
             err,
             "Image '.*' is missing external blobs and is non-functional: .*")
 
+    def _CheckSafeUniqueNames(self, *images):
+        """Check all entries of given images for unsafe unique names"""
+        for image in images:
+            entries = {}
+            image._CollectEntries(entries, {}, image)
+            for entry in entries.values():
+                uniq = entry.GetUniqueName()
+
+                # Used as part of a filename, so must not be absolute paths.
+                self.assertFalse(os.path.isabs(uniq))
+
+    def testSafeUniqueNames(self):
+        """Test entry unique names are safe in single image configuration"""
+        data = self._DoReadFileRealDtb('230_unique_names.dts')
+
+        orig_image = control.images['image']
+        image_fname = tools.get_output_filename('image.bin')
+        image = Image.FromFile(image_fname)
+
+        self._CheckSafeUniqueNames(orig_image, image)
+
+    def testSafeUniqueNamesMulti(self):
+        """Test entry unique names are safe with multiple images"""
+        data = self._DoReadFileRealDtb('231_unique_names_multi.dts')
+
+        orig_image = control.images['image']
+        image_fname = tools.get_output_filename('image.bin')
+        image = Image.FromFile(image_fname)
+
+        self._CheckSafeUniqueNames(orig_image, image)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/230_unique_names.dts b/tools/binman/test/230_unique_names.dts
new file mode 100644
index 000000000000..6780d37f71f4
--- /dev/null
+++ b/tools/binman/test/230_unique_names.dts
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		size = <0xc00>;
+		allow-repack;
+
+		u-boot {
+		};
+
+		fdtmap {
+		};
+
+		u-boot2 {
+			type = "u-boot";
+		};
+
+		text {
+			text = "some text";
+		};
+
+		u-boot-dtb {
+		};
+
+		image-header {
+			location = "end";
+		};
+	};
+};
diff --git a/tools/binman/test/231_unique_names_multi.dts b/tools/binman/test/231_unique_names_multi.dts
new file mode 100644
index 000000000000..db63afb445e0
--- /dev/null
+++ b/tools/binman/test/231_unique_names_multi.dts
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		multiple-images;
+
+		image {
+			size = <0xc00>;
+			allow-repack;
+
+			u-boot {
+			};
+
+			fdtmap {
+			};
+
+			u-boot2 {
+				type = "u-boot";
+			};
+
+			text {
+				text = "some text";
+			};
+
+			u-boot-dtb {
+			};
+
+			image-header {
+				location = "end";
+			};
+		};
+	};
+};
-- 
2.35.1

[PATCH 2/7] binman: Collect bintools for images when replacing entries

[Alper Nebi Yasak]

Binman entries can use other executables to compute their data, usually
in their ObtainContents() methods. Subclasses of Entry_section would use
bintools in their BuildSectionData() method instead, which is called
from several places including their Pack().

These binary tools are resolved correctly while building an image from a
device-tree description so that they can be used from these methods.
However, this is not being done when replacing entries in an image,
which can result in an error as the Pack() methods attempt to use them.

Collect and resolve entries' bintools also when replacing entries to fix
Pack() errors. Add a way to mock bintool usage in the testing entry type
and tests that check bintools are being resolved for such an entry.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/control.py                       |  1 +
 tools/binman/etype/_testing.py                | 36 +++++++++++++++++
 tools/binman/ftest.py                         | 38 ++++++++++++++++++
 .../binman/test/232_replace_with_bintool.dts  | 39 +++++++++++++++++++
 4 files changed, 114 insertions(+)
 create mode 100644 tools/binman/test/232_replace_with_bintool.dts

diff --git a/tools/binman/control.py b/tools/binman/control.py
index d4c8dc89201b..e170aeae4fab 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -299,6 +299,7 @@ def BeforeReplace(image, allow_resize):
     """
     state.PrepareFromLoadedData(image)
     image.LoadData()
+    image.CollectBintools()
 
     # If repacking, drop the old offset/size values except for the original
     # ones, so we are only left with the constraints.
diff --git a/tools/binman/etype/_testing.py b/tools/binman/etype/_testing.py
index 5089de364294..696004878147 100644
--- a/tools/binman/etype/_testing.py
+++ b/tools/binman/etype/_testing.py
@@ -39,6 +39,10 @@ class Entry__testing(Entry):
             error if not)
         force-bad-datatype: Force a call to GetEntryArgsOrProps() with a bad
             data type (generating an error)
+        require-bintool-for-contents: Raise an error if the specified
+            bintool isn't usable in ObtainContents()
+        require-bintool-for-pack: Raise an error if the specified
+            bintool isn't usable in Pack()
     """
     def __init__(self, section, etype, node):
         super().__init__(section, etype, node)
@@ -82,6 +86,26 @@ def ReadNode(self):
         self.return_contents = True
         self.contents = b'aa'
 
+        # Set to the required bintool when collecting bintools.
+        self.bintool_for_contents = None
+        self.require_bintool_for_contents = fdt_util.GetString(self._node,
+                                              'require-bintool-for-contents')
+        if self.require_bintool_for_contents == '':
+            self.require_bintool_for_contents = '_testing'
+
+        self.bintool_for_pack = None
+        self.require_bintool_for_pack = fdt_util.GetString(self._node,
+                                                  'require-bintool-for-pack')
+        if self.require_bintool_for_pack == '':
+            self.require_bintool_for_pack = '_testing'
+
+    def Pack(self, offset):
+        """Figure out how to pack the entry into the section"""
+        if self.require_bintool_for_pack:
+            if self.bintool_for_pack is None:
+                self.Raise("Required bintool unusable in Pack()")
+        return super().Pack(offset)
+
     def ObtainContents(self, fake_size=0):
         if self.return_unknown_contents or not self.return_contents:
             return False
@@ -92,6 +116,9 @@ def ObtainContents(self, fake_size=0):
         self.contents_size = len(self.data)
         if self.return_contents_once:
             self.return_contents = False
+        if self.require_bintool_for_contents:
+            if self.bintool_for_contents is None:
+                self.Raise("Required bintool unusable in ObtainContents()")
         return True
 
     def GetOffsets(self):
@@ -127,3 +154,12 @@ def ProcessFdt(self, fdt):
         if not self.never_complete_process_fdt:
             self.process_fdt_ready = True
         return ready
+
+    def AddBintools(self, btools):
+        """Add the bintools used by this entry type"""
+        if self.require_bintool_for_contents is not None:
+            self.bintool_for_contents = self.AddBintool(btools,
+                    self.require_bintool_for_contents)
+        if self.require_bintool_for_pack is not None:
+            self.bintool_for_pack = self.AddBintool(btools,
+                    self.require_bintool_for_pack)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index e6f0159a229f..da9733d39a6a 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5502,6 +5502,44 @@ def testSafeUniqueNamesMulti(self):
 
         self._CheckSafeUniqueNames(orig_image, image)
 
+    def testReplaceCmdWithBintool(self):
+        """Test replacing an entry that needs a bintool to pack"""
+        data = self._DoReadFileRealDtb('232_replace_with_bintool.dts')
+        expected = U_BOOT_DATA + b'aa'
+        self.assertEqual(expected, data[:len(expected)])
+
+        try:
+            tmpdir, updated_fname = self._SetupImageInTmpdir()
+            fname = os.path.join(tmpdir, 'update-testing.bin')
+            tools.write_file(fname, b'zz')
+            self._DoBinman('replace', '-i', updated_fname,
+                           '_testing', '-f', fname)
+
+            data = tools.read_file(updated_fname)
+            expected = U_BOOT_DATA + b'zz'
+            self.assertEqual(expected, data[:len(expected)])
+        finally:
+            shutil.rmtree(tmpdir)
+
+    def testReplaceCmdOtherWithBintool(self):
+        """Test replacing an entry when another needs a bintool to pack"""
+        data = self._DoReadFileRealDtb('232_replace_with_bintool.dts')
+        expected = U_BOOT_DATA + b'aa'
+        self.assertEqual(expected, data[:len(expected)])
+
+        try:
+            tmpdir, updated_fname = self._SetupImageInTmpdir()
+            fname = os.path.join(tmpdir, 'update-u-boot.bin')
+            tools.write_file(fname, b'x' * len(U_BOOT_DATA))
+            self._DoBinman('replace', '-i', updated_fname,
+                           'u-boot', '-f', fname)
+
+            data = tools.read_file(updated_fname)
+            expected = b'x' * len(U_BOOT_DATA) + b'aa'
+            self.assertEqual(expected, data[:len(expected)])
+        finally:
+            shutil.rmtree(tmpdir)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/232_replace_with_bintool.dts b/tools/binman/test/232_replace_with_bintool.dts
new file mode 100644
index 000000000000..d7fabd2cd835
--- /dev/null
+++ b/tools/binman/test/232_replace_with_bintool.dts
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		size = <0xc00>;
+		allow-repack;
+
+		u-boot {
+		};
+
+		_testing {
+			require-bintool-for-contents;
+			require-bintool-for-pack;
+		};
+
+		fdtmap {
+		};
+
+		u-boot2 {
+			type = "u-boot";
+		};
+
+		text {
+			text = "some text";
+		};
+
+		u-boot-dtb {
+		};
+
+		image-header {
+			location = "end";
+		};
+	};
+};
-- 
2.35.1

[PATCH 3/7] binman: Don't reset offset/size if image doesn't allow repacking

[Alper Nebi Yasak]

When an image has the 'allow-repack' property, binman includes the
original offset and size properties from the image description in the
fdtmap. These are later used as the packing constraints when replacing
entries in an image, so other unconstrained entries can be freely
positioned.

Replacing an entry in an image without 'allow-repack' (and therefore the
original offsets) follows the same logic and results in entries being
merely concatenated. Instead, skip resetting the calculated offsets and
sizes to the missing originals for these images so that every entry is
constrained to its existing offset/size.

Add tests that replace an entry with smaller or equal-sized data, in an
image that doesn't allow repacking. Attempting to do so with bigger-size
data is already an error that is already being tested.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/control.py |  2 +-
 tools/binman/ftest.py   | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/tools/binman/control.py b/tools/binman/control.py
index e170aeae4fab..ce57dc7efc7b 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -303,7 +303,7 @@ def BeforeReplace(image, allow_resize):
 
     # If repacking, drop the old offset/size values except for the original
     # ones, so we are only left with the constraints.
-    if allow_resize:
+    if image.allow_repack and allow_resize:
         image.ResetForPack()
 
 
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index da9733d39a6a..3d79f82dff43 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5540,6 +5540,27 @@ def testReplaceCmdOtherWithBintool(self):
         finally:
             shutil.rmtree(tmpdir)
 
+    def testReplaceResizeNoRepackSameSize(self):
+        """Test replacing entries with same-size data without repacking"""
+        expected = b'x' * len(U_BOOT_DATA)
+        data, expected_fdtmap, _ = self._RunReplaceCmd('u-boot', expected)
+        self.assertEqual(expected, data)
+
+        path, fdtmap = state.GetFdtContents('fdtmap')
+        self.assertIsNotNone(path)
+        self.assertEqual(expected_fdtmap, fdtmap)
+
+    def testReplaceResizeNoRepackSmallerSize(self):
+        """Test replacing entries with smaller-size data without repacking"""
+        new_data = b'x'
+        data, expected_fdtmap, _ = self._RunReplaceCmd('u-boot', new_data)
+        expected = new_data.ljust(len(U_BOOT_DATA), b'\0')
+        self.assertEqual(expected, data)
+
+        path, fdtmap = state.GetFdtContents('fdtmap')
+        self.assertIsNotNone(path)
+        self.assertEqual(expected_fdtmap, fdtmap)
+
 
 if __name__ == "__main__":
     unittest.main()
-- 
2.35.1

[PATCH 4/7] binman: Remove '/images/' fragment from FIT subentry paths

[Alper Nebi Yasak]

Binman FIT entry nodes describe their subentries in an 'images' subnode,
same as how they would be written for the mkimage executable. The entry
type initially manually managed its subentries keyed by their node paths
relative to its base node. It was later converted to a proper section
while still keeping the same keys for subentries.

These subentry keys of sections are used as path fragments, so they must
not contain the path separator character '/'. Otherwise, they won't be
addressable by binman extract/replace commands. Change these keys from
the '/images/foo' forms to the subentry node names. Extend the simple
FIT tests to check for this.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/etype/fit.py | 13 ++++++++-----
 tools/binman/ftest.py     |  7 +++++++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index e0407715d819..035719871e04 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -384,7 +384,8 @@ def _add_entries(base_node, depth, node):
                 entry.ReadNode()
                 # The hash subnodes here are for mkimage, not binman.
                 entry.SetUpdateHash(False)
-                self._entries[rel_path] = entry
+                image_name = rel_path[len('/images/'):]
+                self._entries[image_name] = entry
 
             for subnode in node.subnodes:
                 _add_entries(base_node, depth + 1, subnode)
@@ -630,7 +631,8 @@ def _add_node(base_node, depth, node):
 
             has_images = depth == 2 and in_images
             if has_images:
-                entry = self._priv_entries[rel_path]
+                image_name = rel_path[len('/images/'):]
+                entry = self._priv_entries[image_name]
                 data = entry.GetData()
                 fsw.property('data', bytes(data))
 
@@ -643,12 +645,12 @@ def _add_node(base_node, depth, node):
                     # fsw.add_node() or _add_node() for it.
                     pass
                 elif self.GetImage().generate and subnode.name.startswith('@'):
-                    entry = self._priv_entries.get(subnode_path)
+                    entry = self._priv_entries.get(subnode.name)
                     _gen_node(base_node, subnode, depth, in_images, entry)
                     # This is a generator (template) entry, so remove it from
                     # the list of entries used by PackEntries(), etc. Otherwise
                     # it will appear in the binman output
-                    to_remove.append(subnode_path)
+                    to_remove.append(subnode.name)
                 else:
                     with fsw.add_node(subnode.name):
                         _add_node(base_node, depth + 1, subnode)
@@ -693,7 +695,8 @@ def SetImagePos(self, image_pos):
         fdt = Fdt.FromData(self.GetData())
         fdt.Scan()
 
-        for path, section in self._entries.items():
+        for image_name, section in self._entries.items():
+            path = f"/images/{image_name}"
             node = fdt.GetNode(path)
 
             data_prop = node.props.get("data")
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 3d79f82dff43..87c072ef9c9c 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3761,6 +3761,13 @@ def _CheckSimpleFitData(self, fit_data, kernel_data, fdt1_data):
         self.assertEqual(len(kernel_data), int(data_sizes[0].split()[0]))
         self.assertEqual(len(fdt1_data), int(data_sizes[1].split()[0]))
 
+        # Check if entry listing correctly omits /images/
+        image = control.images['image']
+        fit_entry = image.GetEntries()['fit']
+        subentries = list(fit_entry.GetEntries().keys())
+        expected = ['kernel', 'fdt-1']
+        self.assertEqual(expected, subentries)
+
     def testSimpleFit(self):
         """Test an image with a FIT inside"""
         data = self._DoReadFile('161_fit.dts')
-- 
2.35.1

[PATCH 5/7] binman: Create FIT subentries in the FIT section, not its parent

[Alper Nebi Yasak]

When reading images from a file, each entry's data is read from its
parent section as specified in the Entry.Create() call that created it.
The FIT entry type has been creating its subentries under its parent
(their grandparent), as creating them under the FIT entry resulted in an
error until FIT was converted into a proper section.

FIT subentries have their offsets relative to the FIT section, and
reading those offsets in the parent section results in wrong data. The
subentries rightfully belong under the FIT entries, so create them
there. Add tests checking that we can extract the correct data for a FIT
entry and its subentries.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/etype/fit.py                     |  2 +-
 tools/binman/ftest.py                         | 35 +++++++++
 tools/binman/test/233_fit_extract_replace.dts | 74 +++++++++++++++++++
 3 files changed, 110 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/233_fit_extract_replace.dts

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 035719871e04..12306623af67 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -380,7 +380,7 @@ def _add_entries(base_node, depth, node):
                 # section entries for them here to merge the content subnodes
                 # together and put the merged contents in the subimage node's
                 # 'data' property later.
-                entry = Entry.Create(self.section, node, etype='section')
+                entry = Entry.Create(self, node, etype='section')
                 entry.ReadNode()
                 # The hash subnodes here are for mkimage, not binman.
                 entry.SetUpdateHash(False)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 87c072ef9c9c..a31568997f6f 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5568,6 +5568,41 @@ def testReplaceResizeNoRepackSmallerSize(self):
         self.assertIsNotNone(path)
         self.assertEqual(expected_fdtmap, fdtmap)
 
+    def testExtractFit(self):
+        """Test extracting a FIT section"""
+        self._DoReadFileRealDtb('233_fit_extract_replace.dts')
+        image_fname = tools.get_output_filename('image.bin')
+
+        fit_data = control.ReadEntry(image_fname, 'fit')
+        fit = fdt.Fdt.FromData(fit_data)
+        fit.Scan()
+
+        # Check subentry data inside the extracted fit
+        for node_path, expected in [
+            ('/images/kernel', U_BOOT_DATA),
+            ('/images/fdt-1', U_BOOT_NODTB_DATA),
+            ('/images/scr-1', COMPRESS_DATA),
+        ]:
+            node = fit.GetNode(node_path)
+            data = fit.GetProps(node)['data'].bytes
+            self.assertEqual(expected, data)
+
+    def testExtractFitSubentries(self):
+        """Test extracting FIT section subentries"""
+        self._DoReadFileRealDtb('233_fit_extract_replace.dts')
+        image_fname = tools.get_output_filename('image.bin')
+
+        for entry_path, expected in [
+            ('fit/kernel', U_BOOT_DATA),
+            ('fit/kernel/u-boot', U_BOOT_DATA),
+            ('fit/fdt-1', U_BOOT_NODTB_DATA),
+            ('fit/fdt-1/u-boot-nodtb', U_BOOT_NODTB_DATA),
+            ('fit/scr-1', COMPRESS_DATA),
+            ('fit/scr-1/blob', COMPRESS_DATA),
+        ]:
+            data = control.ReadEntry(image_fname, entry_path)
+            self.assertEqual(expected, data)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/233_fit_extract_replace.dts b/tools/binman/test/233_fit_extract_replace.dts
new file mode 100644
index 000000000000..b44d05afe1af
--- /dev/null
+++ b/tools/binman/test/233_fit_extract_replace.dts
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		allow-repack;
+
+		fill {
+			size = <0x1000>;
+			fill-byte = [77];
+		};
+
+		fit {
+			description = "test-desc";
+			#address-cells = <1>;
+
+			images {
+				kernel {
+					description = "test u-boot";
+					type = "kernel";
+					arch = "arm64";
+					os = "linux";
+					compression = "none";
+					load = <00000000>;
+					entry = <00000000>;
+
+					u-boot {
+					};
+				};
+
+				fdt-1 {
+					description = "test u-boot-nodtb";
+					type = "flat_dt";
+					arch = "arm64";
+					compression = "none";
+
+					u-boot-nodtb {
+					};
+				};
+
+				scr-1 {
+					description = "test blob";
+					type = "script";
+					arch = "arm64";
+					compression = "none";
+
+					blob {
+						filename = "compress";
+					};
+				};
+			};
+
+			configurations {
+				default = "conf-1";
+
+				conf-1 {
+					description = "Kernel with FDT blob";
+					kernel = "kernel";
+					fdt = "fdt-1";
+				};
+			};
+		};
+
+		u-boot-dtb {
+		};
+
+		fdtmap {
+		};
+	};
+};
-- 
2.35.1

[PATCH 6/7] binman: Test replacing non-section entries in FIT subsections

[Alper Nebi Yasak]

A previous patch fixes binman to correctly extract FIT subentries. This
makes it easier to test replacing these entries as we can write tests
using an existing helper function that relies on extracting the replaced
entry.

Add tests that replace leaf entries in FIT subsections with data of
various sizes. Replacing the subsections or the whole FIT section does
not work yet due to the section contents being re-built from unreplaced
subentries' data.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/ftest.py | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index a31568997f6f..43bec4a88841 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5603,6 +5603,44 @@ def testExtractFitSubentries(self):
             data = control.ReadEntry(image_fname, entry_path)
             self.assertEqual(expected, data)
 
+    def testReplaceFitSubentryLeafSameSize(self):
+        """Test replacing a FIT leaf subentry with same-size data"""
+        new_data = b'x' * len(U_BOOT_DATA)
+        data, expected_fdtmap, _ = self._RunReplaceCmd(
+            'fit/kernel/u-boot', new_data,
+            dts='233_fit_extract_replace.dts')
+        self.assertEqual(new_data, data)
+
+        path, fdtmap = state.GetFdtContents('fdtmap')
+        self.assertIsNotNone(path)
+        self.assertEqual(expected_fdtmap, fdtmap)
+
+    def testReplaceFitSubentryLeafBiggerSize(self):
+        """Test replacing a FIT leaf subentry with bigger-size data"""
+        new_data = b'ub' * len(U_BOOT_NODTB_DATA)
+        data, expected_fdtmap, _ = self._RunReplaceCmd(
+            'fit/fdt-1/u-boot-nodtb', new_data,
+            dts='233_fit_extract_replace.dts')
+        self.assertEqual(new_data, data)
+
+        # Will be repacked, so fdtmap must change
+        path, fdtmap = state.GetFdtContents('fdtmap')
+        self.assertIsNotNone(path)
+        self.assertNotEqual(expected_fdtmap, fdtmap)
+
+    def testReplaceFitSubentryLeafSmallerSize(self):
+        """Test replacing a FIT leaf subentry with smaller-size data"""
+        new_data = b'x'
+        expected = new_data.ljust(len(U_BOOT_NODTB_DATA), b'\0')
+        data, expected_fdtmap, _ = self._RunReplaceCmd(
+            'fit/fdt-1/u-boot-nodtb', new_data,
+            dts='233_fit_extract_replace.dts')
+        self.assertEqual(expected, data)
+
+        path, fdtmap = state.GetFdtContents('fdtmap')
+        self.assertIsNotNone(path)
+        self.assertEqual(expected_fdtmap, fdtmap)
+
 
 if __name__ == "__main__":
     unittest.main()
-- 
2.35.1

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

[Alper Nebi Yasak]

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

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

Re: [PATCH 1/7] binman: Fix unique names having '/.' for images read from files

[Simon Glass]

On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Binman can embed a copy of the image description into the images it
> builds as a fdtmap entry, but it omits the /binman/<image-name> prefix
> from the node paths while doing so. When reading an already-built image
> file, entries are reconstructed using this fdtmap and their associated
> nodes still lack that prefix.
>
> Some entries like fit and vblock create intermediate files whose names
> are based on an entry unique name. This name is constructed from their
> node's path by concatenating the parents with dots up to the binman
> node, e.g. /binman/image/foo/bar becomes 'image.foo.bar'.
>
> However, we don't have this /binman/image prefix when replacing entries
> in such an image. The /foo/bar entry we read when doing so erroneously
> has the unique name of '/.foo.bar', causing permission errors when the
> entry attempts to create files based on that.
>
> Fix the unique-name generation by stopping at the '/' node like how it
> stops at the binman node. As the unique names are used as filenames, add
> tests that check if they're safe to use as filenames.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/entry.py                        |  2 +-
>  tools/binman/ftest.py                        | 31 ++++++++++++++++
>  tools/binman/test/230_unique_names.dts       | 34 ++++++++++++++++++
>  tools/binman/test/231_unique_names_multi.dts | 38 ++++++++++++++++++++
>  4 files changed, 104 insertions(+), 1 deletion(-)
>  create mode 100644 tools/binman/test/230_unique_names.dts
>  create mode 100644 tools/binman/test/231_unique_names_multi.dts

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 2/7] binman: Collect bintools for images when replacing entries

[Simon Glass]

On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Binman entries can use other executables to compute their data, usually
> in their ObtainContents() methods. Subclasses of Entry_section would use
> bintools in their BuildSectionData() method instead, which is called
> from several places including their Pack().
>
> These binary tools are resolved correctly while building an image from a
> device-tree description so that they can be used from these methods.
> However, this is not being done when replacing entries in an image,
> which can result in an error as the Pack() methods attempt to use them.
>
> Collect and resolve entries' bintools also when replacing entries to fix
> Pack() errors. Add a way to mock bintool usage in the testing entry type
> and tests that check bintools are being resolved for such an entry.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/control.py                       |  1 +
>  tools/binman/etype/_testing.py                | 36 +++++++++++++++++
>  tools/binman/ftest.py                         | 38 ++++++++++++++++++
>  .../binman/test/232_replace_with_bintool.dts  | 39 +++++++++++++++++++
>  4 files changed, 114 insertions(+)
>  create mode 100644 tools/binman/test/232_replace_with_bintool.dts

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/7] binman: Don't reset offset/size if image doesn't allow repacking

[Simon Glass]

On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> When an image has the 'allow-repack' property, binman includes the
> original offset and size properties from the image description in the
> fdtmap. These are later used as the packing constraints when replacing
> entries in an image, so other unconstrained entries can be freely
> positioned.
>
> Replacing an entry in an image without 'allow-repack' (and therefore the
> original offsets) follows the same logic and results in entries being
> merely concatenated. Instead, skip resetting the calculated offsets and
> sizes to the missing originals for these images so that every entry is
> constrained to its existing offset/size.
>
> Add tests that replace an entry with smaller or equal-sized data, in an
> image that doesn't allow repacking. Attempting to do so with bigger-size
> data is already an error that is already being tested.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/control.py |  2 +-
>  tools/binman/ftest.py   | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 5/7] binman: Create FIT subentries in the FIT section, not its parent

[Simon Glass]

Hi Alper,

On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> When reading images from a file, each entry's data is read from its
> parent section as specified in the Entry.Create() call that created it.
> The FIT entry type has been creating its subentries under its parent
> (their grandparent), as creating them under the FIT entry resulted in an
> error until FIT was converted into a proper section.
>
> FIT subentries have their offsets relative to the FIT section, and
> reading those offsets in the parent section results in wrong data. The
> subentries rightfully belong under the FIT entries, so create them
> there. Add tests checking that we can extract the correct data for a FIT
> entry and its subentries.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/etype/fit.py                     |  2 +-
>  tools/binman/ftest.py                         | 35 +++++++++
>  tools/binman/test/233_fit_extract_replace.dts | 74 +++++++++++++++++++
>  3 files changed, 110 insertions(+), 1 deletion(-)
>  create mode 100644 tools/binman/test/233_fit_extract_replace.dts

Reviewed-by: Simon Glass <sjg@chromium.org>

It's great to be able to replace data in FITs. It's quite a complex
case, but very useful I think.

Regards,
Simon

Re: [PATCH 6/7] binman: Test replacing non-section entries in FIT subsections

[Simon Glass]

On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> A previous patch fixes binman to correctly extract FIT subentries. This
> makes it easier to test replacing these entries as we can write tests
> using an existing helper function that relies on extracting the replaced
> entry.
>
> Add tests that replace leaf entries in FIT subsections with data of
> various sizes. Replacing the subsections or the whole FIT section does
> not work yet due to the section contents being re-built from unreplaced
> subentries' data.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/ftest.py | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

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
>

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

[Alper Nebi Yasak]

On 20/04/2022 00:54, Simon Glass wrote:
> 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?

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