Merge ~ogayot/curtin:sfdisk-accept-utf8 into curtin:master

[Olivier Gayot]

block_meta_v2: make GPT partition name support more robust

    In some cases, the sfdisk script will include GPT partition names. GPT
    partition names support characters outside the ASCII range (they are
    stored in UTF-16 in the GPT metadata). Therefore, it is currently a
    wrong assumption that the sfdisk input is always valid ASCII.

    Furthermore, we inject the name straight inside the sfdisk input script
    (surrounded with double quotes)..
    This can result in various problems if the name contains a '"' character
    (the sfdisk field delimiter) or other characters such as '\\'.

    Fixed by converting all the characters of the partition names into their
    utf-8 \x notation. sfdisk natively supports reading this format. This
    will have a slight implact on the readability of the sfdisk script, but
    should make it way more robust.

udev: fix crash when PARTNAME is invalid utf-8

    (We already merged a similar fix in probert)

    GPT partition names are basically free-text fields but the udev PARTNAME
    field ends up butchered by the kernel when it contains characters
    outside the ASCII range.

    When using pyudev, the values are all expected to be decodable using the
    system's default encoding.

    In curtin, it can result in a crash when iterating over all the
    properties of a block device, that are mapped by pyudev.

    To work around the issue, we now stop iterating over all the values. We
    iterate over the keys and only access the value if we need it.

tests: make random_string a staticmethod instead of classmethod

    CiTestCase.random_string was decorated with @classmethod.
    I guess the purpose was to make the function usable without a CiTestCase
    instance. However, @classmethod is used when a function should return an
    instance of the class.

    This is not what we want. @staticmethod is the right decorator in this
    context.

GPT partition names are basically free-text, but curtin expects the sfdisk input script to be ASCII.

We also inject the partition name in the sfdisk script without properly sanitizing the name. This results in problems if the name contains '"' or other characters that have a special meaning in sfdisk.

Fixed by converting the partition name to a utf-8 hex notation.

e.g.

name="name is réservé"

becomes:

name="name\x20is\x20r\xc3\xa9serv\xc3\xa9"

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:9cace115d30979279febd44f11a47882e3766970
https://jenkins.canonical.com/server-team/job/curtin-ci/100/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/100/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/100/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/100/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/100/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/100//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:851330651c717fdb3fac581b6b6cb73d23a01d94
https://jenkins.canonical.com/server-team/job/curtin-ci/101/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/101/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/101/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/101/
    FAILURE: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/101/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/101//rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6fd3aaceb3e2e4f9e5f8840f4edb7064270e8984
https://jenkins.canonical.com/server-team/job/curtin-ci/102/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/102/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/102/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/102/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/102/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/102//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

Thanks, this all looks reasonable.
An integration test would be nice - I think it should play out similar to test_gpt_uuid_persistent. WDYT?

[Michael Hudson-Doyle (mwhudson)]

Off that was a bit of a brain-o on my part. As Dan says, an integration test would be nice.

[Olivier Gayot (ogayot)]

> Thanks, this all looks reasonable.
An integration test would be nice - I think it should play out similar to test_gpt_uuid_persistent. WDYT?

Thanks! There was test_gpt_name_persistent that existed already. But since you asked, I've added a new test that covers more use-cases (accents + other unicode characters). Hope that's better.

> Off that was a bit of a brain-o on my part. As Dan says, an integration test would be nice.

Done!

NOTE: I'm reluctant to let the CI squash the 3 commits together. If you don't mind, let's merge using the merge guidelines rather than leaving that up to the CI. Thanks!

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2ebbf1ca94b47dbddcf2e94f76356937d9c6e2cc
https://jenkins.canonical.com/server-team/job/curtin-ci/103/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/103/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/103/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/103/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/103/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/103//rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:3969bca45f51057f532272c5666673579e03a59e
https://jenkins.canonical.com/server-team/job/curtin-ci/104/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/104/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/104/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/104/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/104/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/104//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

> I've added a new test that covers more use-cases (accents + other unicode characters). Hope that's better.

Absolutely, thank you.

review: Approve

diff --git a/curtin/commands/block_meta_v2.py b/curtin/commands/block_meta_v2.py
index ae0b058..7f5ebce 100644
--- a/curtin/commands/block_meta_v2.py
+++ b/curtin/commands/block_meta_v2.py
@@ -26,6 +26,23 @@ from curtin.storage_config import (
 from curtin.udev import udevadm_settle
 
 
+def to_utf8_hex_notation(string: str) -> str:
+    ''' Convert a string into a valid ASCII string where all characters outside
+    the alphanumerical range (according to bytes.isalnum()) are translated to
+    their corresponding \\x notation. E.g.:
+        to_utf8_hex_notation("hello") => "hello"
+        to_utf8_hex_notation("réservée") => "r\\xc3\\xa9serv\\xc3\\xa9e"
+        to_utf8_hex_notation("sp ace") => "sp\\x20ace"
+    '''
+    result = ''
+    for c in bytearray(string, 'utf-8'):
+        if bytes([c]).isalnum():
+            result += bytes([c]).decode()
+        else:
+            result += f'\\x{c:02x}'
+    return result
+
+
 @attr.s(auto_attribs=True)
 class PartTableEntry:
     # The order listed here matches the order sfdisk represents these fields
@@ -49,7 +66,11 @@ class PartTableEntry:
             if v is not None:
                 r += ' {}={}'.format(a, v)
         if self.name is not None:
-            r += ' name="{}"'.format(self.name)
+            # Partition names are basically free-text fields. Injecting some
+            # characters such as '"', '\' and '\n' will result in lots of
+            # trouble.  Fortunately, sfdisk supports \x notation, so we can
+            # rely on it.
+            r += ' name="{}"'.format(to_utf8_hex_notation(self.name))
         if self.attrs:
             r += ' attrs="{}"'.format(' '.join(self.attrs))
         if self.bootable:
diff --git a/curtin/udev.py b/curtin/udev.py
index fa4f940..c43e7a5 100644
--- a/curtin/udev.py
+++ b/curtin/udev.py
@@ -131,11 +131,25 @@ def udevadm_info(path=None):
 
 def udev_all_block_device_properties():
     import pyudev
-    props = []
+    devices_props = []
     c = pyudev.Context()
     for device in c.list_devices(subsystem='block'):
-        props.append(dict(device.properties))
-    return props
+        # When dereferencing device[prop], pyudev calls bytes.decode(), which
+        # can fail if the value is invalid utf-8. We don't want a single
+        # invalid value to completely prevent probing. So we iterate
+        # over each value manually and ignore those which are invalid.  We know
+        # that PARTNAME is subject to failures when accents and other special
+        # characters are used in a GPT partition name.
+        # See LP: 2017862
+        props = {}
+        for prop in device.properties:
+            try:
+                props[prop] = device.properties[prop]
+            except UnicodeDecodeError:
+                LOG.warning('ignoring property %s because it is not valid'
+                            ' utf-8', prop)
+        devices_props.append(props)
+    return devices_props
 
 
 # vi: ts=4 expandtab syntax=python
diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
index b25d2ce..d701ca0 100644
--- a/tests/integration/test_block_meta.py
+++ b/tests/integration/test_block_meta.py
@@ -1084,9 +1084,13 @@ class TestBlockMeta(IntegrationTestCase):
             actual_name = sfdisk_info['partitions'][0]['name']
         self.assertEqual(name, actual_name)
 
-    def test_gpt_name_persistent(self):
+    @parameterized.expand([
+        ('random', CiTestCase.random_string(),),
+        # "écrasé" means "overwritten"
+        ('unicode', "'name' must not be écrasé/덮어쓴!"),
+    ])
+    def test_gpt_name_persistent(self, title, name):
         self.img = self.tmp_path('image.img')
-        name = self.random_string()
         config = StorageConfigBuilder(version=2)
         config.add_image(path=self.img, size='20M', ptable='gpt')
         p1 = config.add_part(number=1, offset=1 << 20, size=18 << 20,
diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
index 819e2c5..93b5b5b 100644
--- a/tests/unittests/helpers.py
+++ b/tests/unittests/helpers.py
@@ -152,8 +152,8 @@ class CiTestCase(TestCase):
         return os.path.normpath(
             os.path.abspath(os.path.sep.join((_dir, path))))
 
-    @classmethod
-    def random_string(cls, length=8):
+    @staticmethod
+    def random_string(length=8):
         """ return a random lowercase string with default length of 8"""
         return ''.join(
             random.choice(string.ascii_lowercase) for _ in range(length))
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index dc03359..5599886 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -25,6 +25,34 @@ def random_uuid():
 empty_context = block_meta.BlockMetaContext({})
 
 
+class TestToUTF8HexNotation(CiTestCase):
+    def test_alpha(self):
+        self.assertEqual(
+                block_meta_v2.to_utf8_hex_notation("HelloWorld"), "HelloWorld")
+
+    def test_alphanum(self):
+        self.assertEqual(
+                block_meta_v2.to_utf8_hex_notation("Hello1234"), "Hello1234")
+
+    def test_alnum_space(self):
+        self.assertEqual(
+                block_meta_v2.to_utf8_hex_notation("Hello 1234"),
+                "Hello\\x201234")
+
+    def test_with_accent(self):
+        # '\xe9'.isalpha(), which is equivalent to 'é'.isalpha(), is True
+        # because 'é' is considered a letter according to the unicode standard.
+        # b'\xe9'.isalpha(), on the other hand, is False.
+        self.assertEqual(
+                block_meta_v2.to_utf8_hex_notation("réservée"),
+                "r\\xc3\\xa9serv\\xc3\\xa9e")
+
+    def test_hangul(self):
+        self.assertEqual(
+                block_meta_v2.to_utf8_hex_notation("리눅스"),
+                '\\xeb\\xa6\\xac\\xeb\\x88\\x85\\xec\\x8a\\xa4')
+
+
 class TestGetPathToStorageVolume(CiTestCase):
 
     def setUp(self):
@@ -3050,14 +3078,20 @@ label: gpt
         table.add(dict(number=1, offset=1 << 20, size=9 << 20, flag='boot',
                        partition_name=name))
         type_id = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B"
+        to_hex = block_meta_v2.to_utf8_hex_notation
         expected = f'''\
 label: gpt
 
-1:  start=2048 size=18432 type={type_id} name="{name}"'''
+1:  start=2048 size=18432 type={type_id} name="{to_hex(name)}"'''
         self.assertEqual(expected, table.render())
 
-    def test_gpt_name_spaces(self):
-        name = self.random_string() + " " + self.random_string()
+    def test_gpt_name_free_text(self):
+        name = 'my "분할" réservée'
+        expected_name = ''.join([
+            'my',
+            '\\x20\\x22\\xeb\\xb6\\x84\\xed\\x95\\xa0\\x22',
+            '\\x20r\\xc3\\xa9serv\\xc3\\xa9e',
+        ])
         table = block_meta_v2.GPTPartTable(512)
         table.add(dict(number=1, offset=1 << 20, size=9 << 20, flag='boot',
                        partition_name=name))
@@ -3065,7 +3099,7 @@ label: gpt
         expected = f'''\
 label: gpt
 
-1:  start=2048 size=18432 type={type_id} name="{name}"'''
+1:  start=2048 size=18432 type={type_id} name="{expected_name}"'''
         self.assertEqual(expected, table.render())
 
     def test_gpt_attrs_none(self):
@@ -3255,8 +3289,9 @@ label: dos
                 number=1, start=2, size=3, type='04', bootable=False,
                 uuid=None, name=None, attrs=None)
         pte.preserve({'uuid': uuid, 'name': name, 'attrs': attrs})
+        to_hex = block_meta_v2.to_utf8_hex_notation
         expected = f'1:  start=2 size=3 type=04 uuid={uuid} ' + \
-            f'name="{name}" attrs="{attrs}"'
+            f'name="{to_hex(name)}" attrs="{attrs}"'
         self.assertEqual(expected, pte.render())
 
     def test_v2_dos_is_logical(self):