[pacman-dev] [PATCH 1/5] makepkg: warn about dotfiles in package root
libalpm reserves paths starting with '.' for its own use and will not extract any other than those it recognizes. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- scripts/Makefile.am | 1 + scripts/libmakepkg/lint_package/dotfiles.sh.in | 38 ++++++++++++++++++++++++++ scripts/po/POTFILES.in | 1 + 3 files changed, 40 insertions(+) create mode 100644 scripts/libmakepkg/lint_package/dotfiles.sh.in diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 41a9f5d..4bb08a2 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -58,6 +58,7 @@ LIBMAKEPKG_IN = \ libmakepkg/integrity/verify_signature.sh \ libmakepkg/lint_package.sh \ libmakepkg/lint_package/build_references.sh \ + libmakepkg/lint_package/dotfiles.sh \ libmakepkg/lint_package/file_names.sh \ libmakepkg/lint_package/missing_backup.sh \ libmakepkg/lint_pkgbuild.sh \ diff --git a/scripts/libmakepkg/lint_package/dotfiles.sh.in b/scripts/libmakepkg/lint_package/dotfiles.sh.in new file mode 100644 index 0000000..bf7f41d --- /dev/null +++ b/scripts/libmakepkg/lint_package/dotfiles.sh.in @@ -0,0 +1,38 @@ +#!/bin/bash +# +# dotfiles.sh - check for dotfiles in the package root +# +# Copyright (c) 2016 Pacman Development Team <pacman-dev@archlinux.org> +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +[[ -n "$LIBMAKEPKG_LINT_PACKAGE_DOTFILES_SH" ]] && return +LIBMAKEPKG_LINT_PACKAGE_DOTFILES_SH=1 + +LIBRARY=${LIBRARY:-'@libmakepkgdir@'} + +source "$LIBRARY/util/message.sh" + +lint_package_functions+=('check_dotfiles') + +check_dotfiles() { + local ret=0 + for f in "$pkgdir"/.*; do + [[ ${f##*/} == . || ${f##*/} == .. ]] && continue + error "$(gettext "Dotfile found in package root '%s'")" "$f" + ret=1 + done + return $ret +} diff --git a/scripts/po/POTFILES.in b/scripts/po/POTFILES.in index 4f47fc3..7cc6a03 100644 --- a/scripts/po/POTFILES.in +++ b/scripts/po/POTFILES.in @@ -14,6 +14,7 @@ scripts/libmakepkg/integrity/verify_checksum.sh.in scripts/libmakepkg/integrity/verify_signature.sh.in scripts/libmakepkg/lint_package.sh.in scripts/libmakepkg/lint_package/build_references.sh.in +scripts/libmakepkg/lint_package/dotfiles.sh.in scripts/libmakepkg/lint_package/file_names.sh.in scripts/libmakepkg/lint_package/missing_backup.sh.in scripts/libmakepkg/lint_pkgbuild.sh.in -- 2.10.2
By passing a NUL-separated filelist, this also fixes a bug where files that look like bsdtar options in the package root could break the package ("-C" was particularly troublesome because bsdtar interprets it as an option anywhere in the file list, even following "--"). Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- scripts/makepkg.sh.in | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 2000451..3700307 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -686,6 +686,15 @@ write_buildinfo() { write_kv_pair "installed" "${pkglist[@]}" } +# build a sorted NUL-separated list of the full contents of the current +# directory suitable for passing to `bsdtar --files-from` +# database files are placed at the beginning of the package regardless of +# sorting +list_package_files() { + (find . -path './.*' \! -name '.'; find . \! -path './.*' \! -name '.' | LANG=C sort) | + sed -e 's|^\./||' | tr '\n' '\0' +} + create_package() { (( NOARCHIVE )) && return @@ -702,8 +711,6 @@ create_package() { write_pkginfo > .PKGINFO write_buildinfo > .BUILDINFO - local comp_files=('.PKGINFO' '.BUILDINFO') - # check for changelog/install files for i in 'changelog/.CHANGELOG' 'install/.INSTALL'; do IFS='/' read -r orig dest < <(printf '%s\n' "$i") @@ -715,7 +722,6 @@ create_package() { exit 1 fi chmod 644 "$dest" - comp_files+=("$dest") fi done @@ -727,15 +733,10 @@ create_package() { [[ -f $pkg_file ]] && rm -f "$pkg_file" [[ -f $pkg_file.sig ]] && rm -f "$pkg_file.sig" - # when fileglobbing, we want * in an empty directory to expand to - # the null string rather than itself - shopt -s nullglob - msg2 "$(gettext "Generating .MTREE file...")" - LANG=C bsdtar -czf .MTREE --format=mtree \ + list_package_files | LANG=C bsdtar -cnzf .MTREE --format=mtree \ --options='!all,use-set,type,uid,gid,mode,time,size,md5,sha256,link' \ - "${comp_files[@]}" * - comp_files+=(".MTREE") + --null --files-from - --exclude .MTREE msg2 "$(gettext "Compressing package...")" # TODO: Maybe this can be set globally for robustness @@ -743,7 +744,7 @@ create_package() { # bsdtar's gzip compression always saves the time stamp, making one # archive created using the same command line distinct from another. # Disable bsdtar compression and use gzip -n for now. - LANG=C bsdtar -cf - "${comp_files[@]}" * | + list_package_files | LANG=C bsdtar -cnf - --null --files-from - | case "$PKGEXT" in *tar.gz) ${COMPRESSGZ[@]:-gzip -c -f -n} ;; *tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;; @@ -756,7 +757,6 @@ create_package() { "$PKGEXT"; cat ;; esac > "${pkg_file}" || ret=$? - shopt -u nullglob shopt -u -o pipefail if (( ret )); then -- 2.10.2
On 10/12/16 02:14, Andrew Gregory wrote:
By passing a NUL-separated filelist, this also fixes a bug where files that look like bsdtar options in the package root could break the package ("-C" was particularly troublesome because bsdtar interprets it as an option anywhere in the file list, even following "--").
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- scripts/makepkg.sh.in | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 2000451..3700307 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -686,6 +686,15 @@ write_buildinfo() { write_kv_pair "installed" "${pkglist[@]}" }
+# build a sorted NUL-separated list of the full contents of the current +# directory suitable for passing to `bsdtar --files-from` +# database files are placed at the beginning of the package regardless of +# sorting +list_package_files() { + (find . -path './.*' \! -name '.'; find . \! -path './.*' \! -name '.' | LANG=C sort) | + sed -e 's|^\./||' | tr '\n' '\0' +}
I think we would need LC_ALL or at least LC_COLLATE here. If either is set, LANG will not overwrite them. (I also tried to make this a multi-line function rather than a single line monstrosity but failed printing an array with null delimiters...)
+ create_package() { (( NOARCHIVE )) && return
@@ -702,8 +711,6 @@ create_package() { write_pkginfo > .PKGINFO write_buildinfo > .BUILDINFO
- local comp_files=('.PKGINFO' '.BUILDINFO') - # check for changelog/install files for i in 'changelog/.CHANGELOG' 'install/.INSTALL'; do IFS='/' read -r orig dest < <(printf '%s\n' "$i") @@ -715,7 +722,6 @@ create_package() { exit 1 fi chmod 644 "$dest" - comp_files+=("$dest") fi done
@@ -727,15 +733,10 @@ create_package() { [[ -f $pkg_file ]] && rm -f "$pkg_file" [[ -f $pkg_file.sig ]] && rm -f "$pkg_file.sig"
- # when fileglobbing, we want * in an empty directory to expand to - # the null string rather than itself - shopt -s nullglob - msg2 "$(gettext "Generating .MTREE file...")" - LANG=C bsdtar -czf .MTREE --format=mtree \ + list_package_files | LANG=C bsdtar -cnzf .MTREE --format=mtree \ --options='!all,use-set,type,uid,gid,mode,time,size,md5,sha256,link' \ - "${comp_files[@]}" * - comp_files+=(".MTREE") + --null --files-from - --exclude .MTREE
Why do you need --exclude .MTREE here?
msg2 "$(gettext "Compressing package...")" # TODO: Maybe this can be set globally for robustness @@ -743,7 +744,7 @@ create_package() { # bsdtar's gzip compression always saves the time stamp, making one # archive created using the same command line distinct from another. # Disable bsdtar compression and use gzip -n for now. - LANG=C bsdtar -cf - "${comp_files[@]}" * | + list_package_files | LANG=C bsdtar -cnf - --null --files-from - | case "$PKGEXT" in *tar.gz) ${COMPRESSGZ[@]:-gzip -c -f -n} ;; *tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;; @@ -756,7 +757,6 @@ create_package() { "$PKGEXT"; cat ;; esac > "${pkg_file}" || ret=$?
- shopt -u nullglob shopt -u -o pipefail
if (( ret )); then
On 01/03/17 at 03:09pm, Allan McRae wrote:
On 10/12/16 02:14, Andrew Gregory wrote:
By passing a NUL-separated filelist, this also fixes a bug where files that look like bsdtar options in the package root could break the package ("-C" was particularly troublesome because bsdtar interprets it as an option anywhere in the file list, even following "--").
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- scripts/makepkg.sh.in | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 2000451..3700307 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -686,6 +686,15 @@ write_buildinfo() { write_kv_pair "installed" "${pkglist[@]}" }
+# build a sorted NUL-separated list of the full contents of the current +# directory suitable for passing to `bsdtar --files-from` +# database files are placed at the beginning of the package regardless of +# sorting +list_package_files() { + (find . -path './.*' \! -name '.'; find . \! -path './.*' \! -name '.' | LANG=C sort) | + sed -e 's|^\./||' | tr '\n' '\0' +}
I think we would need LC_ALL or at least LC_COLLATE here. If either is set, LANG will not overwrite them.
Agreed.
(I also tried to make this a multi-line function rather than a single line monstrosity but failed printing an array with null delimiters...)
Yes, it's ugly, but it's POSIX-compliant!
+ create_package() { (( NOARCHIVE )) && return
@@ -702,8 +711,6 @@ create_package() { write_pkginfo > .PKGINFO write_buildinfo > .BUILDINFO
- local comp_files=('.PKGINFO' '.BUILDINFO') - # check for changelog/install files for i in 'changelog/.CHANGELOG' 'install/.INSTALL'; do IFS='/' read -r orig dest < <(printf '%s\n' "$i") @@ -715,7 +722,6 @@ create_package() { exit 1 fi chmod 644 "$dest" - comp_files+=("$dest") fi done
@@ -727,15 +733,10 @@ create_package() { [[ -f $pkg_file ]] && rm -f "$pkg_file" [[ -f $pkg_file.sig ]] && rm -f "$pkg_file.sig"
- # when fileglobbing, we want * in an empty directory to expand to - # the null string rather than itself - shopt -s nullglob - msg2 "$(gettext "Generating .MTREE file...")" - LANG=C bsdtar -czf .MTREE --format=mtree \ + list_package_files | LANG=C bsdtar -cnzf .MTREE --format=mtree \ --options='!all,use-set,type,uid,gid,mode,time,size,md5,sha256,link' \ - "${comp_files[@]}" * - comp_files+=(".MTREE") + --null --files-from - --exclude .MTREE
Why do you need --exclude .MTREE here?
When used with --repackage, the .MTREE file will already exist. Attempting to add the .MTREE file to itself upsets bsdtar. apg
By passing a NUL-separated filelist, this also fixes a bug where files that look like bsdtar options in the package root could break the package ("-C" was particularly troublesome because bsdtar interprets it as an option anywhere in the file list, even following "--"). Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- v2: replace LANG=C with LC_ALL=C for sorting. scripts/makepkg.sh.in | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 20004516..37adff1d 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -686,6 +686,15 @@ write_buildinfo() { write_kv_pair "installed" "${pkglist[@]}" } +# build a sorted NUL-separated list of the full contents of the current +# directory suitable for passing to `bsdtar --files-from` +# database files are placed at the beginning of the package regardless of +# sorting +list_package_files() { + (find . -path './.*' \! -name '.'; find . \! -path './.*' \! -name '.' | LC_ALL=C sort) | + sed -e 's|^\./||' | tr '\n' '\0' +} + create_package() { (( NOARCHIVE )) && return @@ -702,8 +711,6 @@ create_package() { write_pkginfo > .PKGINFO write_buildinfo > .BUILDINFO - local comp_files=('.PKGINFO' '.BUILDINFO') - # check for changelog/install files for i in 'changelog/.CHANGELOG' 'install/.INSTALL'; do IFS='/' read -r orig dest < <(printf '%s\n' "$i") @@ -715,7 +722,6 @@ create_package() { exit 1 fi chmod 644 "$dest" - comp_files+=("$dest") fi done @@ -727,15 +733,10 @@ create_package() { [[ -f $pkg_file ]] && rm -f "$pkg_file" [[ -f $pkg_file.sig ]] && rm -f "$pkg_file.sig" - # when fileglobbing, we want * in an empty directory to expand to - # the null string rather than itself - shopt -s nullglob - msg2 "$(gettext "Generating .MTREE file...")" - LANG=C bsdtar -czf .MTREE --format=mtree \ + list_package_files | LANG=C bsdtar -cnzf .MTREE --format=mtree \ --options='!all,use-set,type,uid,gid,mode,time,size,md5,sha256,link' \ - "${comp_files[@]}" * - comp_files+=(".MTREE") + --null --files-from - --exclude .MTREE msg2 "$(gettext "Compressing package...")" # TODO: Maybe this can be set globally for robustness @@ -743,7 +744,7 @@ create_package() { # bsdtar's gzip compression always saves the time stamp, making one # archive created using the same command line distinct from another. # Disable bsdtar compression and use gzip -n for now. - LANG=C bsdtar -cf - "${comp_files[@]}" * | + list_package_files | LANG=C bsdtar -cnf - --null --files-from - | case "$PKGEXT" in *tar.gz) ${COMPRESSGZ[@]:-gzip -c -f -n} ;; *tar.bz2) ${COMPRESSBZ2[@]:-bzip2 -c -f} ;; @@ -756,7 +757,6 @@ create_package() { "$PKGEXT"; cat ;; esac > "${pkg_file}" || ret=$? - shopt -u nullglob shopt -u -o pipefail if (( ret )); then -- 2.11.0
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- scripts/repo-add.sh.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index a543611..2e921b8 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -455,7 +455,7 @@ db_write_entry() { msg2 "$(gettext "Creating '%s' db entry...")" 'files' local files_path="$tmpdir/files/$pkgname-$pkgver/files" echo "%FILES%" >"$files_path" - bsdtar --exclude='^.*' -tf "$pkgfile" >>"$files_path" + bsdtar --exclude='^.*' -tf "$pkgfile" | LC_ALL=C sort -u >>"$files_path" if (( RMEXISTING )); then msg2 "$(gettext "Removing old package file '%s'")" "$oldfilename" -- 2.10.2
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/be_local.c | 4 +--- lib/libalpm/be_package.c | 3 +-- lib/libalpm/be_sync.c | 3 +-- lib/libalpm/filelist.c | 8 +++++++- lib/libalpm/filelist.h | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c index 15b271f..ad8b245 100644 --- a/lib/libalpm/be_local.c +++ b/lib/libalpm/be_local.c @@ -817,14 +817,12 @@ static int local_db_read(alpm_pkg_t *info, int inforeq) if(newfiles != NULL) { files = newfiles; } - - /* make sure the list is sorted */ - qsort(files, files_count, sizeof(alpm_file_t), _alpm_files_cmp); } else { FREE(files); } info->files.count = files_count; info->files.files = files; + _alpm_filelist_sort(&info->files); continue; nomem: while(files_count > 0) { diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index b7c54fa..926edbf 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -676,8 +676,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, _alpm_log(handle, ALPM_LOG_DEBUG, "sorting package filelist for %s\n", pkgfile); - qsort(newpkg->files.files, newpkg->files.count, - sizeof(alpm_file_t), _alpm_files_cmp); + _alpm_filelist_sort(&newpkg->files); } newpkg->infolevel |= INFRQ_FILES; } diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 7774975..edf4746 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -735,13 +735,12 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, /* attempt to hand back any memory we don't need */ if(files_count > 0) { files = realloc(files, sizeof(alpm_file_t) * files_count); - /* make sure the list is sorted */ - qsort(files, files_count, sizeof(alpm_file_t), _alpm_files_cmp); } else { FREE(files); } pkg->files.count = files_count; pkg->files.files = files; + _alpm_filelist_sort(&pkg->files); } } if(ret != ARCHIVE_EOF) { diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 5783373..318b2b7 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -111,7 +111,7 @@ alpm_list_t *_alpm_filelist_intersection(alpm_filelist_t *filesA, /* Helper function for comparing files list entries */ -int _alpm_files_cmp(const void *f1, const void *f2) +static int _alpm_files_cmp(const void *f1, const void *f2) { const alpm_file_t *file1 = f1; const alpm_file_t *file2 = f2; @@ -133,4 +133,10 @@ alpm_file_t SYMEXPORT *alpm_filelist_contains(alpm_filelist_t *filelist, sizeof(alpm_file_t), _alpm_files_cmp); } +void _alpm_filelist_sort(alpm_filelist_t *filelist) +{ + qsort(filelist->files, filelist->count, + sizeof(alpm_file_t), _alpm_files_cmp); +} + /* vim: set noet: */ diff --git a/lib/libalpm/filelist.h b/lib/libalpm/filelist.h index a74bdea..fa6d5c5 100644 --- a/lib/libalpm/filelist.h +++ b/lib/libalpm/filelist.h @@ -27,7 +27,7 @@ alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, alpm_list_t *_alpm_filelist_intersection(alpm_filelist_t *filesA, alpm_filelist_t *filesB); -int _alpm_files_cmp(const void *f1, const void *f2); +void _alpm_filelist_sort(alpm_filelist_t *filelist); #endif /* ALPM_FILELIST_H */ -- 2.10.2
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- This patch obviously won't do any good until packages start being built with their file lists presorted, so we may want to defer it, but in the handful of packages I tested, it did not cause a significant delay either. lib/libalpm/filelist.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 318b2b7..f6fdf37 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -135,8 +135,15 @@ alpm_file_t SYMEXPORT *alpm_filelist_contains(alpm_filelist_t *filelist, void _alpm_filelist_sort(alpm_filelist_t *filelist) { - qsort(filelist->files, filelist->count, - sizeof(alpm_file_t), _alpm_files_cmp); + size_t i; + for(i = 1; i < filelist->count; i++) { + if(strcmp(filelist->files[i - 1].name, filelist->files[i].name) > 0) { + /* filelist is not pre-sorted */ + qsort(filelist->files, filelist->count, + sizeof(alpm_file_t), _alpm_files_cmp); + return; + } + } } /* vim: set noet: */ -- 2.10.2
On 10/12/16 02:14, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
This patch obviously won't do any good until packages start being built with their file lists presorted, so we may want to defer it, but in the handful of packages I tested, it did not cause a significant delay either.
Would it be best to just recommend that repo databases are regenerated with the updated repo-add? Not including this would result in the qsort operations on the pre-sorted file lists having the worst possible performance. A
On 01/03/17 at 02:20pm, Allan McRae wrote:
On 10/12/16 02:14, Andrew Gregory wrote:
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> ---
This patch obviously won't do any good until packages start being built with their file lists presorted, so we may want to defer it, but in the handful of packages I tested, it did not cause a significant delay either.
Would it be best to just recommend that repo databases are regenerated with the updated repo-add? Not including this would result in the qsort operations on the pre-sorted file lists having the worst possible performance.
I wouldn't expect pre-sorting to degrade performance for modern qsort implementations (it certainly didn't for me), but the performance hit for unsorted dbs/packages is probably modest enough that it's better to just go ahead and merge this. apg
On 10/12/16 02:14, Andrew Gregory wrote:
libalpm reserves paths starting with '.' for its own use and will not extract any other than those it recognizes.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com>
OK.
participants (2)
-
Allan McRae
-
Andrew Gregory