| Message ID | 20180219201145.14057-6-eschwartz@archlinux.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix ambiguous uses of $PKGEXT | expand |
On Mon, 19 Feb 2018 15:11:45 -0500, Eli Schwartz via arch-projects wrote: > Document the fact that this has *always* been some sort of glob, and > update the two cases where this was (not!) being evaluated by bash > [[ ... ]], to use a not-elegant-at-all proxy function is_globfile() to > evaluate globs *before* testing if they exist. ... > @@ -378,8 +383,8 @@ check_pkgrepos() { > local pkgver="$(getpkgver ${pkgfile})" || return 1 > local pkgarch="$(getpkgarch ${pkgfile})" || return 1 > > - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXTS} ]] && return 1 > - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXTS}.sig ]] && return 1 > + is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXTS} && return 1 > + is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXTS}.sig && return 1 > [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/} ]] && return 1 > [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/}.sig ]] && return 1 It's not a big deal, but I'd rather that be a separate commit, as it's fixing breakage that's unrelated to switching it from a plain glob to an extglob. > diff --git a/config b/config > index 5bb3b16..0d33de0 100644 > --- a/config > +++ b/config > @@ -25,7 +25,8 @@ TMPDIR="/var/tmp" > ARCHES=(x86_64) > DBEXT=".db.tar.gz" > FILESEXT=".files.tar.gz" > -PKGEXTS=".pkg.tar.?z" > +# bash glob listing allowed extensions. Note that db-functions turns on extglob. > +PKGEXTS=".pkg.tar.@(gz|bz2|xz|lzo|lrz|Z)" > SRCEXT=".src.tar.gz" > Is there a reason you reject '.pkg.tar' (no compression, which makepkg accepts)? (I also found it curious that you swapped lzo and lrz from the order the extensions are in in the makepkg source.) (Also, I'd move it down a line, so that it's more obvious that the comment doesn't apply to SRCEXT.)
On 02/19/2018 04:59 PM, Luke Shumaker wrote: > Is there a reason you reject '.pkg.tar' (no compression, which makepkg > accepts)? I don't think there is any utility in supporting uncompressed packages in dbscripts. Anyone who wants to customize this in a non-Arch Linux deployment is free to do so... If someone wants to use some deviant compression type because they're positive it works better on those packaged files, I cannot think of a compelling reason to say "no you're wrong", which is why I listed everything else. > (I also found it curious that you swapped lzo and lrz from the order > the extensions are in in the makepkg source.) makepkg is inconsistent here, I pulled that from the makepkg.conf(5) source. :D
On Mon, 19 Feb 2018 15:11:45 -0500, Eli Schwartz via arch-projects wrote: > +# Check if a file exists, even if the file uses wildcards > +is_globfile() { > + [[ -f $1 ]] > +} > + Dave's comment on my version of this patchset applies equally to this version: > Frankly, this function name and comment sucks, because it says nothing > about quoting. As I read the comment, I'm lead to believe that given a > file "foobar" existing, I can call: __isGlobfile "foo*", and this will > succeed. To the naive reader, you might even believe this claim based on > the unquotedness of $1 within the -f test. (I had the same function, with the same comment, as is_globfile in db-functions and as __isGlobfile in common.bash)
diff --git a/config b/config index 5bb3b16..0d33de0 100644 --- a/config +++ b/config @@ -25,7 +25,8 @@ TMPDIR="/var/tmp" ARCHES=(x86_64) DBEXT=".db.tar.gz" FILESEXT=".files.tar.gz" -PKGEXTS=".pkg.tar.?z" +# bash glob listing allowed extensions. Note that db-functions turns on extglob. +PKGEXTS=".pkg.tar.@(gz|bz2|xz|lzo|lrz|Z)" SRCEXT=".src.tar.gz" # Allowed licenses: get sourceballs only for licenses in this array diff --git a/db-functions b/db-functions index 394c7a2..e36d43b 100644 --- a/db-functions +++ b/db-functions @@ -3,7 +3,7 @@ . /usr/share/makepkg/util.sh # global shell options for enhanced bash scripting -shopt -s globstar nullglob +shopt -s extglob globstar nullglob # Some PKGBUILDs need CARCH to be set @@ -20,6 +20,11 @@ restore_umask () { umask $UMASK >/dev/null } +# Check if a file exists, even if the file uses wildcards +is_globfile() { + [[ -f $1 ]] +} + # just like mv -f, but we touch the file and then copy the content so # default ACLs in the target dir will be applied mv_acl() { @@ -378,8 +383,8 @@ check_pkgrepos() { local pkgver="$(getpkgver ${pkgfile})" || return 1 local pkgarch="$(getpkgarch ${pkgfile})" || return 1 - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXTS} ]] && return 1 - [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}${PKGEXTS}.sig ]] && return 1 + is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXTS} && return 1 + is_globfile "${FTP_BASE}/${PKGPOOL}/${pkgname}-${pkgver}-${pkgarch}"${PKGEXTS}.sig && return 1 [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/} ]] && return 1 [[ -f ${FTP_BASE}/${PKGPOOL}/${pkgfile##*/}.sig ]] && return 1
The current glob `*.pkg.tar.?z` is both less restrictive and more restrictive than makepkg, as it accepts any valid unicode character. To be more exact, it's almost completely orthogonal to the one in makepkg. makepkg only accepts .tar.gz, .tar.bz2, .tar.xz, .tar.lzo, .tar.lrz, and .tar.Z and most of those fail to match against a two-char compression type. dbscripts accepts .pkg.tar.💩z which incidentally is what I think of cherry-picking xz and gz as supported methods. Since this can be anything makepkg.conf accepts, it needs to be able to match all that, unless we decide to perform additional restrictions in which case we should still explicitly list each allowed extension. Using bash extended globbing allows us to do this relatively painlessly. Document the fact that this has *always* been some sort of glob, and update the two cases where this was (not!) being evaluated by bash [[ ... ]], to use a not-elegant-at-all proxy function is_globfile() to evaluate globs *before* testing if they exist. Signed-off-by: Eli Schwartz <eschwartz@archlinux.org> --- No changes, just a reworded commit message. config | 3 ++- db-functions | 11 ++++++++--- 2 files changed, 10 insertions(+), 4 deletions(-)