binman: Fix unique names having '/.' for images read from files

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>
Reviewed-by: Simon Glass <sjg@chromium.org>

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 18a7a3510548552f9277471e44cea32eeed4172e..a07a5888643a70d1a34c8a5564895da38047f6ac 100644 (file)
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -775,7 +775,7 @@ features to produce new behaviours.
         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 4ce181a06663f835128dee999dbc4d93fd6fb1ac..94c4389b1b52fb419f34d28db53ab5b28dcf3351 100644 (file)
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5523,5 +5523,36 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         with self.assertRaises(ValueError) as e:
             data = self._DoReadFile('231_pre_load_invalid_key.dts')
 
+    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 (file)
index 0000000..6780d37
--- /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 (file)
index 0000000..db63afb
--- /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";
+                       };
+               };
+       };
+};