[PATCH] fetch2: fix handling of `\` in file:// SRC_URI

[PATCH] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

Using backslashes in file:// URIs was broken.
Either the resolver would fail or the subsequent `cp` command.
Try to avoid this by putting the filenames into quotes.

Fixes https://bugzilla.yoctoproject.org/show_bug.cgi?id=8161

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/fetch2/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 7ec1fea5..1911ecd3 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1520,7 +1520,7 @@ class FetchMethod(object):
                     if urlpath.find("/") != -1:
                         destdir = urlpath.rsplit("/", 1)[0] + '/'
                         bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
-                cmd = 'cp -fpPRH %s %s' % (file, destdir)
+                cmd = "cp -fpPRH '%s' '%s'" % (file, destdir)
 
         if not cmd:
             return
-- 
2.26.2

Re: [bitbake-devel] [PATCH] fetch2: fix handling of `\` in file:// SRC_URI

[Richard Purdie]

On Wed, 2020-09-23 at 15:32 +0200, Leif Middelschulte wrote:
> Using backslashes in file:// URIs was broken.
> Either the resolver would fail or the subsequent `cp` command.
> Try to avoid this by putting the filenames into quotes.
> 
> Fixes https://bugzilla.yoctoproject.org/show_bug.cgi?id=8161
> 
> Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
> ---
>  lib/bb/fetch2/__init__.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
> index 7ec1fea5..1911ecd3 100644
> --- a/lib/bb/fetch2/__init__.py
> +++ b/lib/bb/fetch2/__init__.py
> @@ -1520,7 +1520,7 @@ class FetchMethod(object):
>                      if urlpath.find("/") != -1:
>                          destdir = urlpath.rsplit("/", 1)[0] + '/'
>                          bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
> -                cmd = 'cp -fpPRH %s %s' % (file, destdir)
> +                cmd = "cp -fpPRH '%s' '%s'" % (file, destdir)
>  
>          if not cmd:
>              return

Thanks for the fix. Would you mind adding a test to
lib/bb/tests/fetch.py (run with bitbake-selftest) so we can catch this
issue in future please?

Cheers,

Richard

[PATCH v2 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

Using backslashes in file:// URIs was broken.
Either the resolver would fail or the subsequent `cp` command.
Try to avoid this by putting the filenames into quotes.

Fixes https://bugzilla.yoctoproject.org/show_bug.cgi?id=8161

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/fetch2/__init__.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 7ec1fea5..6bf70337 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -21,6 +21,7 @@ if 'git' not in urllib.parse.uses_netloc:
 import operator
 import collections
 import subprocess
+import glob
 import pickle
 import errno
 import bb.persist_data, bb.utils
@@ -1520,7 +1521,9 @@ class FetchMethod(object):
                     if urlpath.find("/") != -1:
                         destdir = urlpath.rsplit("/", 1)[0] + '/'
                         bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
-                cmd = 'cp -fpPRH %s %s' % (file, destdir)
+                # do not rely on Shell's glob expansion.
+                file = ' '.join("'{}'".format(file_path) for file_path in glob.glob(file))
+                cmd = 'cp -fpPRH %s "%s"' % (file, destdir)
 
         if not cmd:
             return
-- 
2.26.2

Re: [bitbake-devel] [PATCH v2 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Enrico Scholz]

"Leif Middelschulte" <leif.middelschulte@klsmartin.com> writes:

> -                cmd = 'cp -fpPRH %s %s' % (file, destdir)
> +                # do not rely on Shell's glob expansion.
> +                file = ' '.join("'{}'".format(file_path) for file_path in glob.glob(file))
> +                cmd = 'cp -fpPRH %s "%s"' % (file, destdir)

Why single quotes in "file" but double quotes in the cmd?


Would it make sense to implement this correctly from the beginning.  E.g.

- avoid manual quoting but use "pipes.quote()"

- do not use a shell command string but an array which is given directly
  to execve()?

Else, next patchset will deal with single/double quotes...



Enrico

Re: [bitbake-devel] [PATCH v2 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

Am Montag, den 05.10.2020, 15:29 +0200 schrieb Enrico Scholz:
> CAUTION: This message was sent from outside of KLS Martin. Please do not click links or open attachments unless you recognize the source of this email and know the content is safe.
> 
> "Leif Middelschulte" <
> leif.middelschulte@klsmartin.com
> > writes:
> 
> > -                cmd = 'cp -fpPRH %s %s' % (file, destdir)
> > +                # do not rely on Shell's glob expansion.
> > +                file = ' '.join("'{}'".format(file_path) for file_path in glob.glob(file))
> > +                cmd = 'cp -fpPRH %s "%s"' % (file, destdir)
> 
> Why single quotes in "file" but double quotes in the cmd?
You are correct. It should be double quotes too, to allow variable expansion by the shell.
The latest patchset revision uses double quotes for the source too.
> 
> 
> Would it make sense to implement this correctly from the beginning.  E.g.
> 
> - avoid manual quoting but use "pipes.quote()"
I assume this would be shlex.quotes?
> 
> - do not use a shell command string but an array which is given directly
>   to execve()?
AFAII it's desired to be able to reuse other environment variables in `SRC_URI`s.
What's more is that once https://bugs.python.org/issue38893 is fixed, one might want to use python
native copy operations anyway.
> 
> Else, next patchset will deal with single/double quotes...

Globbing support was removed. This patchset revision became kind of obsolete.
> 
> 
> 
> Enrico

Thanks for the feedback,

Leif

Re: [bitbake-devel] [PATCH v2 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Enrico Scholz]

"Middelschulte, Leif" <Leif.Middelschulte@klsmartin.com> writes:

>> > +                file = ' '.join("'{}'".format(file_path) for file_path in glob.glob(file))
>> > +                cmd = 'cp -fpPRH %s "%s"' % (file, destdir)
>> 
>> Why single quotes in "file" but double quotes in the cmd?
>
> You are correct. It should be double quotes too, to allow variable
> expansion by the shell.

Is it really allowed/expected to have environment variables in SRC_URI?
E.g. is it expected that

| SRC_URI = "file://$HOME/foo"

works?  This fails here because other parts of bitbake (lookup in
${FILESPATH} or in MIRRORS) use this string directly.


>> Would it make sense to implement this correctly from the beginning.  E.g.
>> 
>> - avoid manual quoting but use "pipes.quote()"
> I assume this would be shlex.quotes?

ok; yes.  It is shlex in python3


>> - do not use a shell command string but an array which is given directly
>>   to execve()?
>
> AFAII it's desired to be able to reuse other environment variables in
> `SRC_URI`s.  What's more is that once https://bugs.python.org/issue38893
> is fixed, one might want to use python

But native python operations do not expand environment variables
either.  Resp. expansion must be done manually which is possible with
the "execve([])" way too.



Enrico

[PATCH v2 2/2] tests/fetch: backslash support in file:// URIs

[Leif Middelschulte]

Implements backslashes in local filenames.
A typical usecase for such a filename is a systemd unit.

Example: `dev-disk-by\x2dpath-foo.device`
---
 lib/bb/tests/fetch.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 0ecf044f..c392afc9 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -584,6 +584,7 @@ class FetcherLocalTest(FetcherTest):
         touch(os.path.join(self.localsrcdir, 'dir', 'd'))
         os.makedirs(os.path.join(self.localsrcdir, 'dir', 'subdir'))
         touch(os.path.join(self.localsrcdir, 'dir', 'subdir', 'e'))
+        touch(os.path.join(self.localsrcdir, 'backslash\x2dsystemd-unit.device'))
         self.d.setVar("FILESPATH", self.localsrcdir)
 
     def fetchUnpack(self, uris):
@@ -601,6 +602,10 @@ class FetcherLocalTest(FetcherTest):
         tree = self.fetchUnpack(['file://a', 'file://dir/c'])
         self.assertEqual(tree, ['a', 'dir/c'])
 
+    def test_local_backslash(self):
+        tree = self.fetchUnpack(['file://backslash\\x2dsystemd-unit.device'])
+        self.assertEqual(tree, ['backslash\\x2dsystemd-unit.device'])
+
     def test_local_wildcard(self):
         with self.assertRaises(bb.fetch2.ParameterError):
             tree = self.fetchUnpack(['file://a', 'file://dir/*'])
-- 
2.26.2

Re: [bitbake-devel] [PATCH v2 2/2] tests/fetch: backslash support in file:// URIs

[Richard Purdie]

On Wed, 2020-09-23 at 18:18 +0200, Leif Middelschulte wrote:
> Implements backslashes in local filenames.
> A typical usecase for such a filename is a systemd unit.
> 
> Example: `dev-disk-by\x2dpath-foo.device`
> ---
>  lib/bb/tests/fetch.py | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
> index 0ecf044f..c392afc9 100644
> --- a/lib/bb/tests/fetch.py
> +++ b/lib/bb/tests/fetch.py
> @@ -584,6 +584,7 @@ class FetcherLocalTest(FetcherTest):
>          touch(os.path.join(self.localsrcdir, 'dir', 'd'))
>          os.makedirs(os.path.join(self.localsrcdir, 'dir', 'subdir'))
>          touch(os.path.join(self.localsrcdir, 'dir', 'subdir', 'e'))
> +        touch(os.path.join(self.localsrcdir, 'backslash\x2dsystemd-unit.device'))
>          self.d.setVar("FILESPATH", self.localsrcdir)
>  
>      def fetchUnpack(self, uris):
> @@ -601,6 +602,10 @@ class FetcherLocalTest(FetcherTest):
>          tree = self.fetchUnpack(['file://a', 'file://dir/c'])
>          self.assertEqual(tree, ['a', 'dir/c'])
>  
> +    def test_local_backslash(self):
> +        tree = self.fetchUnpack(['file://backslash\\x2dsystemd-unit.device'])
> +        self.assertEqual(tree, ['backslash\\x2dsystemd-unit.device'])
> +
>      def test_local_wildcard(self):
>          with self.assertRaises(bb.fetch2.ParameterError):
>              tree = self.fetchUnpack(['file://a', 'file://dir/*'])

Thanks for this, looks good. Unfortunately something doesn't seem quite
right as the tests fail:

https://autobuilder.yoctoproject.org/typhoon/#/builders/79/builds/1395/steps/8/logs/step1d

?

Cheers,

Richard

[PATCH v3 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

Using backslashes in file:// URIs was broken.
Either the resolver would fail or the subsequent `cp` command.
Try to avoid this by putting the filenames into quotes.

Fixes https://bugzilla.yoctoproject.org/show_bug.cgi?id=8161

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/fetch2/__init__.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 7ec1fea5..6bf70337 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -21,6 +21,7 @@ if 'git' not in urllib.parse.uses_netloc:
 import operator
 import collections
 import subprocess
+import glob
 import pickle
 import errno
 import bb.persist_data, bb.utils
@@ -1520,7 +1521,9 @@ class FetchMethod(object):
                     if urlpath.find("/") != -1:
                         destdir = urlpath.rsplit("/", 1)[0] + '/'
                         bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
-                cmd = 'cp -fpPRH %s %s' % (file, destdir)
+                # do not rely on Shell's glob expansion.
+                file = ' '.join("'{}'".format(file_path) for file_path in glob.glob(file))
+                cmd = 'cp -fpPRH %s "%s"' % (file, destdir)
 
         if not cmd:
             return
-- 
2.26.2

[PATCH v3 2/2] tests/fetch: backslash support in file:// URIs

[Leif Middelschulte]

Implements backslashes in local filenames.
A typical usecase for such a filename is a systemd unit.

Example: `dev-disk-by\x2dpath-foo.device`
---
 lib/bb/tests/fetch.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 0ecf044f..9030d6d9 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -584,6 +584,7 @@ class FetcherLocalTest(FetcherTest):
         touch(os.path.join(self.localsrcdir, 'dir', 'd'))
         os.makedirs(os.path.join(self.localsrcdir, 'dir', 'subdir'))
         touch(os.path.join(self.localsrcdir, 'dir', 'subdir', 'e'))
+        touch(os.path.join(self.localsrcdir, 'backslash\\x2dsystemd-unit.device'))
         self.d.setVar("FILESPATH", self.localsrcdir)
 
     def fetchUnpack(self, uris):
@@ -601,6 +602,10 @@ class FetcherLocalTest(FetcherTest):
         tree = self.fetchUnpack(['file://a', 'file://dir/c'])
         self.assertEqual(tree, ['a', 'dir/c'])
 
+    def test_local_backslash(self):
+        tree = self.fetchUnpack(['file://backslash\\x2dsystemd-unit.device'])
+        self.assertEqual(tree, ['backslash\\x2dsystemd-unit.device'])
+
     def test_local_wildcard(self):
         with self.assertRaises(bb.fetch2.ParameterError):
             tree = self.fetchUnpack(['file://a', 'file://dir/*'])
-- 
2.26.2

[PATCH v4] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

This patch series allows users to specify `file://` SRC_URIs containing backslashes (`\`).

Patch series revision v1:
- Provide simple (using quotes around paths) patch to allow backslahes.

Patch series revision v2:
- Rework simple patch to handle glob expansion inside Python instead of relying on the Shell.
- Provide second patch to test `file://` SRC_URIs containing backslashes.

Patch series revision v3:
- Fixup typo in python filename specification.

Patch series revision v4:
- Add missing DCO for patch 2/2.

[PATCH v4 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

Using backslashes in file:// URIs was broken.
Either the resolver would fail or the subsequent `cp` command.
Try to avoid this by putting the filenames into quotes.

Fixes https://bugzilla.yoctoproject.org/show_bug.cgi?id=8161

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/fetch2/__init__.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 7ec1fea5..6bf70337 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -21,6 +21,7 @@ if 'git' not in urllib.parse.uses_netloc:
 import operator
 import collections
 import subprocess
+import glob
 import pickle
 import errno
 import bb.persist_data, bb.utils
@@ -1520,7 +1521,9 @@ class FetchMethod(object):
                     if urlpath.find("/") != -1:
                         destdir = urlpath.rsplit("/", 1)[0] + '/'
                         bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
-                cmd = 'cp -fpPRH %s %s' % (file, destdir)
+                # do not rely on Shell's glob expansion.
+                file = ' '.join("'{}'".format(file_path) for file_path in glob.glob(file))
+                cmd = 'cp -fpPRH %s "%s"' % (file, destdir)
 
         if not cmd:
             return
-- 
2.26.2

[PATCH v4 2/2] tests/fetch: backslash support in file:// URIs

[Leif Middelschulte]

Implements backslashes in local filenames.
A typical usecase for such a filename is a systemd unit.

Example: `dev-disk-by\x2dlabel-FOO.device`

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/tests/fetch.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 0ecf044f..9030d6d9 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -584,6 +584,7 @@ class FetcherLocalTest(FetcherTest):
         touch(os.path.join(self.localsrcdir, 'dir', 'd'))
         os.makedirs(os.path.join(self.localsrcdir, 'dir', 'subdir'))
         touch(os.path.join(self.localsrcdir, 'dir', 'subdir', 'e'))
+        touch(os.path.join(self.localsrcdir, 'backslash\\x2dsystemd-unit.device'))
         self.d.setVar("FILESPATH", self.localsrcdir)
 
     def fetchUnpack(self, uris):
@@ -601,6 +602,10 @@ class FetcherLocalTest(FetcherTest):
         tree = self.fetchUnpack(['file://a', 'file://dir/c'])
         self.assertEqual(tree, ['a', 'dir/c'])
 
+    def test_local_backslash(self):
+        tree = self.fetchUnpack(['file://backslash\\x2dsystemd-unit.device'])
+        self.assertEqual(tree, ['backslash\\x2dsystemd-unit.device'])
+
     def test_local_wildcard(self):
         with self.assertRaises(bb.fetch2.ParameterError):
             tree = self.fetchUnpack(['file://a', 'file://dir/*'])
-- 
2.26.2

[PATCH v5 1/2] fetch2: fix handling of `\` in file:// SRC_URI

[Leif Middelschulte]

Using backslashes in file:// URIs was broken.
Either the resolver would fail or the subsequent `cp` command.
Try to avoid this by putting the filenames into quotes.

Fixes https://bugzilla.yoctoproject.org/show_bug.cgi?id=8161

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/fetch2/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/bb/fetch2/__init__.py b/lib/bb/fetch2/__init__.py
index 7ec1fea5..551bfb70 100644
--- a/lib/bb/fetch2/__init__.py
+++ b/lib/bb/fetch2/__init__.py
@@ -1520,7 +1520,7 @@ class FetchMethod(object):
                     if urlpath.find("/") != -1:
                         destdir = urlpath.rsplit("/", 1)[0] + '/'
                         bb.utils.mkdirhier("%s/%s" % (unpackdir, destdir))
-                cmd = 'cp -fpPRH %s %s' % (file, destdir)
+                cmd = 'cp -fpPRH "%s" "%s"' % (file, destdir)
 
         if not cmd:
             return
-- 
2.26.2

[PATCH v5 2/2] tests/fetch: backslash support in file:// URIs

[Leif Middelschulte]

Implements backslashes in local filenames.
A typical usecase for such a filename is a systemd unit.

Example: `dev-disk-by\x2dlabel-FOO.device`

Signed-off-by: Leif Middelschulte <leif.middelschulte@klsmartin.com>
---
 lib/bb/tests/fetch.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/bb/tests/fetch.py b/lib/bb/tests/fetch.py
index 0ecf044f..c078cd5b 100644
--- a/lib/bb/tests/fetch.py
+++ b/lib/bb/tests/fetch.py
@@ -584,6 +584,7 @@ class FetcherLocalTest(FetcherTest):
         touch(os.path.join(self.localsrcdir, 'dir', 'd'))
         os.makedirs(os.path.join(self.localsrcdir, 'dir', 'subdir'))
         touch(os.path.join(self.localsrcdir, 'dir', 'subdir', 'e'))
+        touch(os.path.join(self.localsrcdir, r'backslash\x2dsystemd-unit.device'))
         self.d.setVar("FILESPATH", self.localsrcdir)
 
     def fetchUnpack(self, uris):
@@ -601,6 +602,10 @@ class FetcherLocalTest(FetcherTest):
         tree = self.fetchUnpack(['file://a', 'file://dir/c'])
         self.assertEqual(tree, ['a', 'dir/c'])
 
+    def test_local_backslash(self):
+        tree = self.fetchUnpack([r'file://backslash\x2dsystemd-unit.device'])
+        self.assertEqual(tree, [r'backslash\x2dsystemd-unit.device'])
+
     def test_local_wildcard(self):
         with self.assertRaises(bb.fetch2.ParameterError):
             tree = self.fetchUnpack(['file://a', 'file://dir/*'])
-- 
2.26.2