diff mbox series

classes/package_rpm: correctly escape percent characters

Message ID 20240129180629.1631473-1-alex@linutronix.de
State Accepted, archived
Commit 6d9fe2623c37e405a80acf71633f7291ecdde533
Headers show
Series classes/package_rpm: correctly escape percent characters | expand

Commit Message

Alexander Kanavin Jan. 29, 2024, 6:06 p.m. UTC
This many characters doesn't work with rpm 4.19 packaging
(as shown by nodejs recipes), and per documentation a single escape
is enough:

https://github.com/rpm-software-management/rpm/blob/rpm-4.19.x/docs/manual/spec.md#shell-globbing

It also should be done in a function, and just before writing out the
corrected filename to .spec, not earlier where the path may still
be needed for file operations (such as gettings file attributes).

Signed-off-by: Alexander Kanavin <alex@linutronix.de>
---
 meta/classes-global/package_rpm.bbclass | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Mark Hatle Jan. 29, 2024, 6:42 p.m. UTC | #1
This looks better to me.  Hopefully two % is all we need now.

Thanks!
--Mark

On 1/29/24 12:06 PM, Alexander Kanavin wrote:
> This many characters doesn't work with rpm 4.19 packaging
> (as shown by nodejs recipes), and per documentation a single escape
> is enough:
> 
> https://github.com/rpm-software-management/rpm/blob/rpm-4.19.x/docs/manual/spec.md#shell-globbing
> 
> It also should be done in a function, and just before writing out the
> corrected filename to .spec, not earlier where the path may still
> be needed for file operations (such as gettings file attributes).
> 
> Signed-off-by: Alexander Kanavin <alex@linutronix.de>
> ---
>   meta/classes-global/package_rpm.bbclass | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
> index e0f4de42a15..819ee502783 100644
> --- a/meta/classes-global/package_rpm.bbclass
> +++ b/meta/classes-global/package_rpm.bbclass
> @@ -216,10 +216,12 @@ python write_specfile () {
>                       raise e
>                   return "%attr({:o},{},{}) ".format(mode, owner, group)
>   
> +            def escape_chars(p):
> +                return p.replace("%", "%%")
> +
>               path = rootpath.replace(walkpath, "")
>               if path.endswith("DEBIAN") or path.endswith("CONTROL"):
>                   continue
> -            path = path.replace("%", "%%%%%%%%")
>   
>               # Treat all symlinks to directories as normal files.
>               # os.walk() lists them as directories.
> @@ -238,29 +240,27 @@ python write_specfile () {
>                   for dir in dirs:
>                       if dir == "CONTROL" or dir == "DEBIAN":
>                           continue
> -                    dir = dir.replace("%", "%%%%%%%%")
>                       p = path + '/' + dir
>                       # All packages own the directories their files are in...
> -                    target.append(get_attr(dir) + '%dir "' + p + '"')
> +                    target.append(get_attr(dir) + '%dir "' + escape_chars(p) + '"')
>               else:
>                   # packages own only empty directories or explict directory.
>                   # This will prevent the overlapping of security permission.
>                   attr = get_attr(path)
>                   if path and not files and not dirs:
> -                    target.append(attr + '%dir "' + path + '"')
> +                    target.append(attr + '%dir "' + escape_chars(path) + '"')
>                   elif path and path in dirfiles:
> -                    target.append(attr + '%dir "' + path + '"')
> +                    target.append(attr + '%dir "' + escape_chars(path) + '"')
>   
>               for file in files:
>                   if file == "CONTROL" or file == "DEBIAN":
>                       continue
> -                file = file.replace("%", "%%%%%%%%")
>                   attr = get_attr(file)
>                   p = path + '/' + file
>                   if conffiles.count(p):
> -                    target.append(attr + '%config "' + p + '"')
> +                    target.append(attr + '%config "' + escape_chars(p) + '"')
>                   else:
> -                    target.append(attr + '"' + p + '"')
> +                    target.append(attr + '"' + escape_chars(p) + '"')
>   
>       # Prevent the prerm/postrm scripts from being run during an upgrade
>       def wrap_uninstall(scriptvar):
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#194473): https://lists.openembedded.org/g/openembedded-core/message/194473
> Mute This Topic: https://lists.openembedded.org/mt/104036885/3616948
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [mark.hatle@kernel.crashing.org]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Peter Kjellerstedt Jan. 31, 2024, 1:07 a.m. UTC | #2
> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Alexander Kanavin
> Sent: den 29 januari 2024 19:06
> To: openembedded-core@lists.openembedded.org
> Cc: Alexander Kanavin <alex@linutronix.de>
> Subject: [OE-core] [PATCH] classes/package_rpm: correctly escape percent characters
> 
> This many characters doesn't work with rpm 4.19 packaging
> (as shown by nodejs recipes), and per documentation a single escape
> is enough:
> 
> https://github.com/rpm-software-management/rpm/blob/rpm-4.19.x/docs/manual/spec.md#shell-globbing
> 
> It also should be done in a function, and just before writing out the
> corrected filename to .spec, not earlier where the path may still
> be needed for file operations (such as gettings file attributes).
> 
> Signed-off-by: Alexander Kanavin <alex@linutronix.de>
> ---
>  meta/classes-global/package_rpm.bbclass | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
> index e0f4de42a15..819ee502783 100644
> --- a/meta/classes-global/package_rpm.bbclass
> +++ b/meta/classes-global/package_rpm.bbclass
> @@ -216,10 +216,12 @@ python write_specfile () {
>                      raise e
>                  return "%attr({:o},{},{}) ".format(mode, owner, group)
> 
> +            def escape_chars(p):
> +                return p.replace("%", "%%")


Based on the documentation linked to above, wouldn't it be a good idea to 
handle \ and " as well? E.g.:

                return p.replace("%", "%%").replace("\\", "\\\\").replace('"', '\\"'))

> +
>              path = rootpath.replace(walkpath, "")
>              if path.endswith("DEBIAN") or path.endswith("CONTROL"):
>                  continue
> -            path = path.replace("%", "%%%%%%%%")
> 
>              # Treat all symlinks to directories as normal files.
>              # os.walk() lists them as directories.
> @@ -238,29 +240,27 @@ python write_specfile () {
>                  for dir in dirs:
>                      if dir == "CONTROL" or dir == "DEBIAN":
>                          continue
> -                    dir = dir.replace("%", "%%%%%%%%")
>                      p = path + '/' + dir
>                      # All packages own the directories their files are in...
> -                    target.append(get_attr(dir) + '%dir "' + p + '"')
> +                    target.append(get_attr(dir) + '%dir "' + escape_chars(p) + '"')
>              else:
>                  # packages own only empty directories or explict directory.
>                  # This will prevent the overlapping of security permission.
>                  attr = get_attr(path)
>                  if path and not files and not dirs:
> -                    target.append(attr + '%dir "' + path + '"')
> +                    target.append(attr + '%dir "' + escape_chars(path) + '"')
>                  elif path and path in dirfiles:
> -                    target.append(attr + '%dir "' + path + '"')
> +                    target.append(attr + '%dir "' + escape_chars(path) + '"')

The above can be simplified to:

            elif path:
                # packages own only empty directories or explict directory.
                # This will prevent the overlapping of security permission.
                attr = get_attr(path)
                if (not files and not dirs) or path in dirfiles:
                    target.append(attr + '%dir "' + escape_chars(path) + '"')

> 
>              for file in files:
>                  if file == "CONTROL" or file == "DEBIAN":
>                      continue
> -                file = file.replace("%", "%%%%%%%%")
>                  attr = get_attr(file)
>                  p = path + '/' + file
>                  if conffiles.count(p):
> -                    target.append(attr + '%config "' + p + '"')
> +                    target.append(attr + '%config "' + escape_chars(p) + '"')
>                  else:
> -                    target.append(attr + '"' + p + '"')
> +                    target.append(attr + '"' + escape_chars(p) + '"')
> 
>      # Prevent the prerm/postrm scripts from being run during an upgrade
>      def wrap_uninstall(scriptvar):
> --
> 2.39.2

//Peter
Alexander Kanavin Jan. 31, 2024, 12:04 p.m. UTC | #3
Thanks, I sent patches for both.

Alex

On Wed, 31 Jan 2024 at 02:07, Peter Kjellerstedt
<peter.kjellerstedt@axis.com> wrote:
>
> > -----Original Message-----
> > From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Alexander Kanavin
> > Sent: den 29 januari 2024 19:06
> > To: openembedded-core@lists.openembedded.org
> > Cc: Alexander Kanavin <alex@linutronix.de>
> > Subject: [OE-core] [PATCH] classes/package_rpm: correctly escape percent characters
> >
> > This many characters doesn't work with rpm 4.19 packaging
> > (as shown by nodejs recipes), and per documentation a single escape
> > is enough:
> >
> > https://github.com/rpm-software-management/rpm/blob/rpm-4.19.x/docs/manual/spec.md#shell-globbing
> >
> > It also should be done in a function, and just before writing out the
> > corrected filename to .spec, not earlier where the path may still
> > be needed for file operations (such as gettings file attributes).
> >
> > Signed-off-by: Alexander Kanavin <alex@linutronix.de>
> > ---
> >  meta/classes-global/package_rpm.bbclass | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
> > index e0f4de42a15..819ee502783 100644
> > --- a/meta/classes-global/package_rpm.bbclass
> > +++ b/meta/classes-global/package_rpm.bbclass
> > @@ -216,10 +216,12 @@ python write_specfile () {
> >                      raise e
> >                  return "%attr({:o},{},{}) ".format(mode, owner, group)
> >
> > +            def escape_chars(p):
> > +                return p.replace("%", "%%")
>
>
> Based on the documentation linked to above, wouldn't it be a good idea to
> handle \ and " as well? E.g.:
>
>                 return p.replace("%", "%%").replace("\\", "\\\\").replace('"', '\\"'))
>
> > +
> >              path = rootpath.replace(walkpath, "")
> >              if path.endswith("DEBIAN") or path.endswith("CONTROL"):
> >                  continue
> > -            path = path.replace("%", "%%%%%%%%")
> >
> >              # Treat all symlinks to directories as normal files.
> >              # os.walk() lists them as directories.
> > @@ -238,29 +240,27 @@ python write_specfile () {
> >                  for dir in dirs:
> >                      if dir == "CONTROL" or dir == "DEBIAN":
> >                          continue
> > -                    dir = dir.replace("%", "%%%%%%%%")
> >                      p = path + '/' + dir
> >                      # All packages own the directories their files are in...
> > -                    target.append(get_attr(dir) + '%dir "' + p + '"')
> > +                    target.append(get_attr(dir) + '%dir "' + escape_chars(p) + '"')
> >              else:
> >                  # packages own only empty directories or explict directory.
> >                  # This will prevent the overlapping of security permission.
> >                  attr = get_attr(path)
> >                  if path and not files and not dirs:
> > -                    target.append(attr + '%dir "' + path + '"')
> > +                    target.append(attr + '%dir "' + escape_chars(path) + '"')
> >                  elif path and path in dirfiles:
> > -                    target.append(attr + '%dir "' + path + '"')
> > +                    target.append(attr + '%dir "' + escape_chars(path) + '"')
>
> The above can be simplified to:
>
>             elif path:
>                 # packages own only empty directories or explict directory.
>                 # This will prevent the overlapping of security permission.
>                 attr = get_attr(path)
>                 if (not files and not dirs) or path in dirfiles:
>                     target.append(attr + '%dir "' + escape_chars(path) + '"')
>
> >
> >              for file in files:
> >                  if file == "CONTROL" or file == "DEBIAN":
> >                      continue
> > -                file = file.replace("%", "%%%%%%%%")
> >                  attr = get_attr(file)
> >                  p = path + '/' + file
> >                  if conffiles.count(p):
> > -                    target.append(attr + '%config "' + p + '"')
> > +                    target.append(attr + '%config "' + escape_chars(p) + '"')
> >                  else:
> > -                    target.append(attr + '"' + p + '"')
> > +                    target.append(attr + '"' + escape_chars(p) + '"')
> >
> >      # Prevent the prerm/postrm scripts from being run during an upgrade
> >      def wrap_uninstall(scriptvar):
> > --
> > 2.39.2
>
> //Peter
>
diff mbox series

Patch

diff --git a/meta/classes-global/package_rpm.bbclass b/meta/classes-global/package_rpm.bbclass
index e0f4de42a15..819ee502783 100644
--- a/meta/classes-global/package_rpm.bbclass
+++ b/meta/classes-global/package_rpm.bbclass
@@ -216,10 +216,12 @@  python write_specfile () {
                     raise e
                 return "%attr({:o},{},{}) ".format(mode, owner, group)
 
+            def escape_chars(p):
+                return p.replace("%", "%%")
+
             path = rootpath.replace(walkpath, "")
             if path.endswith("DEBIAN") or path.endswith("CONTROL"):
                 continue
-            path = path.replace("%", "%%%%%%%%")
 
             # Treat all symlinks to directories as normal files.
             # os.walk() lists them as directories.
@@ -238,29 +240,27 @@  python write_specfile () {
                 for dir in dirs:
                     if dir == "CONTROL" or dir == "DEBIAN":
                         continue
-                    dir = dir.replace("%", "%%%%%%%%")
                     p = path + '/' + dir
                     # All packages own the directories their files are in...
-                    target.append(get_attr(dir) + '%dir "' + p + '"')
+                    target.append(get_attr(dir) + '%dir "' + escape_chars(p) + '"')
             else:
                 # packages own only empty directories or explict directory.
                 # This will prevent the overlapping of security permission.
                 attr = get_attr(path)
                 if path and not files and not dirs:
-                    target.append(attr + '%dir "' + path + '"')
+                    target.append(attr + '%dir "' + escape_chars(path) + '"')
                 elif path and path in dirfiles:
-                    target.append(attr + '%dir "' + path + '"')
+                    target.append(attr + '%dir "' + escape_chars(path) + '"')
 
             for file in files:
                 if file == "CONTROL" or file == "DEBIAN":
                     continue
-                file = file.replace("%", "%%%%%%%%")
                 attr = get_attr(file)
                 p = path + '/' + file
                 if conffiles.count(p):
-                    target.append(attr + '%config "' + p + '"')
+                    target.append(attr + '%config "' + escape_chars(p) + '"')
                 else:
-                    target.append(attr + '"' + p + '"')
+                    target.append(attr + '"' + escape_chars(p) + '"')
 
     # Prevent the prerm/postrm scripts from being run during an upgrade
     def wrap_uninstall(scriptvar):