[PATCH] libmakepkg/integrity: fix regression that broke --install

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] libmakepkg/integrity: fix regression that broke --install

Eli Schwartz-2
In commit c6b04c04653ba9933fe978829148312e412a9ea7 package signing was
moved out of fakeroot, and as part of this process, the global pkgname
variable was modified in order to extract the built package names.

However, if a debug package was not available and added to the list of
packages, the function was aborted early, before the pkgname array was
restored, thereby corrupting the later stages of makepkg and
specifically the install_package function which needs to know which
pkgnames to install.

Fix this by inlining the debug package signing inside the `if` check,
and as added security switch to using `for pkg in "${pkgname[@]}"` as is
done in many other parts of makepkg, since package signing does not
depend on the value of pkgname for anything.

Signed-off-by: Eli Schwartz <[hidden email]>
---
 .../libmakepkg/integrity/generate_signature.sh.in    | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/scripts/libmakepkg/integrity/generate_signature.sh.in b/scripts/libmakepkg/integrity/generate_signature.sh.in
index 8bb69984..c8b938ab 100644
--- a/scripts/libmakepkg/integrity/generate_signature.sh.in
+++ b/scripts/libmakepkg/integrity/generate_signature.sh.in
@@ -50,28 +50,24 @@ create_package_signatures() {
  if [[ $SIGNPKG != 'y' ]]; then
  return 0
  fi
- local pkgarch pkg_file
+ local pkg pkgarch pkg_file
  local pkgname_backup=("${pkgname[@]}")
  local fullver=$(get_full_version)
 
  msg "$(gettext "Signing package(s)...")"
 
- for pkgname in ${pkgname_backup[@]}; do
- pkgarch=$(get_pkg_arch $pkgname)
- pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
+ for pkg in "${pkgname[@]}"; do
+ pkgarch=$(get_pkg_arch $pkg)
+ pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"
 
  create_signature "$pkg_file"
  done
 
  # check if debug package needs a signature
  if ! check_option "debug" "y" || ! check_option "strip" "y"; then
- return
+ pkg=$pkgbase-@DEBUGSUFFIX@
+ pkgarch=$(get_pkg_arch)
+ pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"
+ create_signature "$pkg_file"
  fi
-
- pkgname=$pkgbase-@DEBUGSUFFIX@
- pkgarch=$(get_pkg_arch)
- pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
- create_signature "$pkg_file"
-
- pkgname=("${pkgname_backup[@]}")
 }
--
2.16.2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] libmakepkg/integrity: fix regression that broke --install

Eli Schwartz-2
On 03/05/2018 10:36 AM, Eli Schwartz wrote:

> - local pkgarch pkg_file
> + local pkg pkgarch pkg_file
>   local pkgname_backup=("${pkgname[@]}")
>   local fullver=$(get_full_version)
>  
>   msg "$(gettext "Signing package(s)...")"
>  
> - for pkgname in ${pkgname_backup[@]}; do
> - pkgarch=$(get_pkg_arch $pkgname)
> - pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
> + for pkg in "${pkgname[@]}"; do
> + pkgarch=$(get_pkg_arch $pkg)
> + pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"
>  
>   create_signature "$pkg_file"
>   done
>  
>   # check if debug package needs a signature
>   if ! check_option "debug" "y" || ! check_option "strip" "y"; then
BTW any reason we even need to do all this instead of, say, using
print_all_package_names

We could probably do that both here and in install_package, especially
if/when "makepkg --packagelist: just list the built package files we
will build" is accepted.

See? This patch is even useful for makepkg itself, why would we ever not
want it. :p

--
Eli Schwartz
Bug Wrangler and Trusted User


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] libmakepkg/integrity: fix regression that broke --install

Allan McRae
In reply to this post by Eli Schwartz-2
On 06/03/18 01:36, Eli Schwartz wrote:

> In commit c6b04c04653ba9933fe978829148312e412a9ea7 package signing was
> moved out of fakeroot, and as part of this process, the global pkgname
> variable was modified in order to extract the built package names.
>
> However, if a debug package was not available and added to the list of
> packages, the function was aborted early, before the pkgname array was
> restored, thereby corrupting the later stages of makepkg and
> specifically the install_package function which needs to know which
> pkgnames to install.
>
> Fix this by inlining the debug package signing inside the `if` check,
> and as added security switch to using `for pkg in "${pkgname[@]}"` as is
> done in many other parts of makepkg, since package signing does not
> depend on the value of pkgname for anything.
>
> Signed-off-by: Eli Schwartz <[hidden email]>
> ---
>  .../libmakepkg/integrity/generate_signature.sh.in    | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/scripts/libmakepkg/integrity/generate_signature.sh.in b/scripts/libmakepkg/integrity/generate_signature.sh.in
> index 8bb69984..c8b938ab 100644
> --- a/scripts/libmakepkg/integrity/generate_signature.sh.in
> +++ b/scripts/libmakepkg/integrity/generate_signature.sh.in
> @@ -50,28 +50,24 @@ create_package_signatures() {
>   if [[ $SIGNPKG != 'y' ]]; then
>   return 0
>   fi
> - local pkgarch pkg_file
> + local pkg pkgarch pkg_file
>   local pkgname_backup=("${pkgname[@]}")

This variable is no longer needed.

>   local fullver=$(get_full_version)
>  
>   msg "$(gettext "Signing package(s)...")"
>  
> - for pkgname in ${pkgname_backup[@]}; do
> - pkgarch=$(get_pkg_arch $pkgname)
> - pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
> + for pkg in "${pkgname[@]}"; do
> + pkgarch=$(get_pkg_arch $pkg)
> + pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"
>  
>   create_signature "$pkg_file"
>   done
>  
>   # check if debug package needs a signature
>   if ! check_option "debug" "y" || ! check_option "strip" "y"; then
> - return
> + pkg=$pkgbase-@DEBUGSUFFIX@
> + pkgarch=$(get_pkg_arch)
> + pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"

We should check this file exists.   The create_signature function will
still fail when the package is not there, which can happen if there is
no binaries in the package. (e.g. arch=any packages).

> + create_signature "$pkg_file"
>   fi
> -
> - pkgname=$pkgbase-@DEBUGSUFFIX@
> - pkgarch=$(get_pkg_arch)
> - pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
> - create_signature "$pkg_file"
> -
> - pkgname=("${pkgname_backup[@]}")
>  }
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] libmakepkg/integrity: fix regression that broke --install

Eli Schwartz-2
On 03/14/2018 01:34 AM, Allan McRae wrote:

>>   # check if debug package needs a signature
>>   if ! check_option "debug" "y" || ! check_option "strip" "y"; then
>> - return
>> + pkg=$pkgbase-@DEBUGSUFFIX@
>> + pkgarch=$(get_pkg_arch)
>> + pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"
>
> We should check this file exists.   The create_signature function will
> still fail when the package is not there, which can happen if there is
> no binaries in the package. (e.g. arch=any packages).
True -- looks like this was broken for a while now (before
create_package_signatures this was actually run inside the debug
packaging run).

--
Eli Schwartz
Bug Wrangler and Trusted User


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] libmakepkg/integrity: fix regression that broke --install

Eli Schwartz-2
In reply to this post by Allan McRae
In commit c6b04c04653ba9933fe978829148312e412a9ea7 package signing was
moved out of fakeroot, and as part of this process, the global pkgname
variable was modified in order to extract the built package names.

However, if a debug package was not available and added to the list of
packages, the function was aborted early, before the pkgname array was
restored, thereby corrupting the later stages of makepkg and
specifically the install_package function which needs to know which
pkgnames to install.

Fix this by inlining the debug package signing inside the `if` check,
and as added security switch to using `for pkg in "${pkgname[@]}"` as is
done in many other parts of makepkg, since package signing does not
depend on the value of pkgname for anything.

Additionally, since debug packages may not actually exist, check if the
package file exists first.

Signed-off-by: Eli Schwartz <[hidden email]>
---

v2: rm unused variable, also fix case where debug packages aren't
created because no debug symbols were found.

 .../libmakepkg/integrity/generate_signature.sh.in  | 23 ++++++++++------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/scripts/libmakepkg/integrity/generate_signature.sh.in b/scripts/libmakepkg/integrity/generate_signature.sh.in
index 8bb69984..785792bb 100644
--- a/scripts/libmakepkg/integrity/generate_signature.sh.in
+++ b/scripts/libmakepkg/integrity/generate_signature.sh.in
@@ -50,28 +50,25 @@ create_package_signatures() {
  if [[ $SIGNPKG != 'y' ]]; then
  return 0
  fi
- local pkgarch pkg_file
- local pkgname_backup=("${pkgname[@]}")
+ local pkg pkgarch pkg_file
  local fullver=$(get_full_version)

  msg "$(gettext "Signing package(s)...")"

- for pkgname in ${pkgname_backup[@]}; do
- pkgarch=$(get_pkg_arch $pkgname)
- pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
+ for pkg in "${pkgname[@]}"; do
+ pkgarch=$(get_pkg_arch $pkg)
+ pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"

  create_signature "$pkg_file"
  done

  # check if debug package needs a signature
  if ! check_option "debug" "y" || ! check_option "strip" "y"; then
- return
+ pkg=$pkgbase-@DEBUGSUFFIX@
+ pkgarch=$(get_pkg_arch)
+ pkg_file="$PKGDEST/${pkg}-${fullver}-${pkgarch}${PKGEXT}"
+ if [[ -f $pkg_file ]]; then
+ create_signature "$pkg_file"
+ fi
  fi
-
- pkgname=$pkgbase-@DEBUGSUFFIX@
- pkgarch=$(get_pkg_arch)
- pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
- create_signature "$pkg_file"
-
- pkgname=("${pkgname_backup[@]}")
 }
--
2.16.2
Reply | Threaded
Open this post in threaded view
|

[PATCH v3] libmakepkg/integrity: fix regression that broke --install

Eli Schwartz-2
In reply to this post by Eli Schwartz-2
In commit c6b04c04653ba9933fe978829148312e412a9ea7 package signing was
moved out of fakeroot, and as part of this process, the global pkgname
variable was modified in order to extract the built package names.

However, if a debug package was not available and added to the list of
packages, the function was aborted early, before the pkgname array was
restored, thereby corrupting the later stages of makepkg and
specifically the install_package function which needs to know which
pkgnames to install.

Fix this by using the newly changed print_all_package_names function to
iterate over the list of all package files that will be created; this
avoids the need to independently recreate those filenames here.

Additionally, since debug packages may not actually exist, check if the
package file exists first.

Signed-off-by: Eli Schwartz <[hidden email]>
---

v3: alternative take that relies on the --packagelist changes currently
in Allan's patchqueue. Deduplicating this logic feels nice...

 .../libmakepkg/integrity/generate_signature.sh.in  | 24 ++++------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/scripts/libmakepkg/integrity/generate_signature.sh.in b/scripts/libmakepkg/integrity/generate_signature.sh.in
index 8bb69984..032e147e 100644
--- a/scripts/libmakepkg/integrity/generate_signature.sh.in
+++ b/scripts/libmakepkg/integrity/generate_signature.sh.in
@@ -50,28 +50,12 @@ create_package_signatures() {
  if [[ $SIGNPKG != 'y' ]]; then
  return 0
  fi
- local pkgarch pkg_file
- local pkgname_backup=("${pkgname[@]}")
- local fullver=$(get_full_version)

  msg "$(gettext "Signing package(s)...")"

- for pkgname in ${pkgname_backup[@]}; do
- pkgarch=$(get_pkg_arch $pkgname)
- pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
-
- create_signature "$pkg_file"
+ print_all_package_names | while read pkg_file; do
+ if [[ -f $pkg_file ]]; then
+ create_signature "$pkg_file"
+ fi
  done
-
- # check if debug package needs a signature
- if ! check_option "debug" "y" || ! check_option "strip" "y"; then
- return
- fi
-
- pkgname=$pkgbase-@DEBUGSUFFIX@
- pkgarch=$(get_pkg_arch)
- pkg_file="$PKGDEST/${pkgname}-${fullver}-${pkgarch}${PKGEXT}"
- create_signature "$pkg_file"
-
- pkgname=("${pkgname_backup[@]}")
 }
--
2.16.2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] libmakepkg/integrity: fix regression that broke --install

Allan McRae
In reply to this post by Eli Schwartz-2
On 15/03/18 10:42, Eli Schwartz wrote:

> In commit c6b04c04653ba9933fe978829148312e412a9ea7 package signing was
> moved out of fakeroot, and as part of this process, the global pkgname
> variable was modified in order to extract the built package names.
>
> However, if a debug package was not available and added to the list of
> packages, the function was aborted early, before the pkgname array was
> restored, thereby corrupting the later stages of makepkg and
> specifically the install_package function which needs to know which
> pkgnames to install.
>
> Fix this by inlining the debug package signing inside the `if` check,
> and as added security switch to using `for pkg in "${pkgname[@]}"` as is
> done in many other parts of makepkg, since package signing does not
> depend on the value of pkgname for anything.
>
> Additionally, since debug packages may not actually exist, check if the
> package file exists first.
>
> Signed-off-by: Eli Schwartz <[hidden email]>
> ---
>
> v2: rm unused variable, also fix case where debug packages aren't
> created because no debug symbols were found.
>

Looks good!

A