[pacman-dev] Fwd: [PATCH] repo-add: fix eval and quote issues
eval was ugly and dirty, and bit us here. Instead, use a safer form of variable declaration to ensure quotes don't foil us in pkgdesc or any other fields.
This fixes FS#10837.
Signed-off-by: Dan McGee <dan@archlinux.org> --- We apparently had never handled quotes in descriptions correctly, so
Yes, I think I sent this to myself on accident. Looks like it is time for another cup of coffee. ---------- Forwarded message ---------- From: Dan McGee <dan@archlinux.org> Date: Sun, Feb 8, 2009 at 12:30 PM Subject: Re: [PATCH] repo-add: fix eval and quote issues To: Dan McGee <dan@archlinux.org> On Sun, Feb 8, 2009 at 12:22 PM, Dan McGee <dan@archlinux.org> wrote: this patch should fix these issues. I'm going to push this patch to maint if there are no objections, although I expect our next release will come from master.
scripts/repo-add.sh.in | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index be0859e..93fdd52 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -152,18 +152,19 @@ db_write_entry()
# read info from the zipped package local line I did make one small fix here- added var and val to the local declaration. - for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do - eval "$line" - case "$line" in - group=*) _groups="$_groups$group\n" ;; - depend=*) _depends="$_depends$depend\n" ;; - backup=*) _backups="$_backups$backup\n" ;; - license=*) _licenses="$_licenses$license\n" ;; - replaces=*) _replaces="$_replaces$replaces\n" ;; - provides=*) _provides="$_provides$provides\n" ;; - conflict=*) _conflicts="$_conflicts$conflict\n" ;; - optdepend=*) _optdepends="$_optdepends$optdepend\n" ;; + for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | grep -v '^#'); do + var="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\1|')" + val="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\2|')" + declare $var="$val" + case "$var" in + group) _groups="$_groups$group\n" ;; + depend) _depends="$_depends$depend\n" ;; + backup) _backups="$_backups$backup\n" ;; + license) _licenses="$_licenses$license\n" ;; + replaces) _replaces="$_replaces$replaces\n" ;; + provides) _provides="$_provides$provides\n" ;; + conflict) _conflicts="$_conflicts$conflict\n" ;; + optdepend) _optdepends="$_optdepends$optdepend\n" ;; esac done
-- 1.6.1.2
On Sun, Feb 8, 2009 at 7:31 PM, Dan McGee <dan@archlinux.org> wrote:
Yes, I think I sent this to myself on accident. Looks like it is time for another cup of coffee.
---------- Forwarded message ---------- From: Dan McGee <dan@archlinux.org> Date: Sun, Feb 8, 2009 at 12:30 PM Subject: Re: [PATCH] repo-add: fix eval and quote issues To: Dan McGee <dan@archlinux.org>
eval was ugly and dirty, and bit us here. Instead, use a safer form of variable declaration to ensure quotes don't foil us in pkgdesc or any other fields.
This fixes FS#10837.
Signed-off-by: Dan McGee <dan@archlinux.org> --- We apparently had never handled quotes in descriptions correctly, so
On Sun, Feb 8, 2009 at 12:22 PM, Dan McGee <dan@archlinux.org> wrote: this patch should fix these issues. I'm going to push this patch to maint if there are no objections, although I expect our next release will come from master.
scripts/repo-add.sh.in | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index be0859e..93fdd52 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -152,18 +152,19 @@ db_write_entry()
# read info from the zipped package local line I did make one small fix here- added var and val to the local declaration. - for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do - eval "$line" - case "$line" in - group=*) _groups="$_groups$group\n" ;; - depend=*) _depends="$_depends$depend\n" ;; - backup=*) _backups="$_backups$backup\n" ;; - license=*) _licenses="$_licenses$license\n" ;; - replaces=*) _replaces="$_replaces$replaces\n" ;; - provides=*) _provides="$_provides$provides\n" ;; - conflict=*) _conflicts="$_conflicts$conflict\n" ;; - optdepend=*) _optdepends="$_optdepends$optdepend\n" ;; + for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | grep -v '^#'); do + var="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\1|')" + val="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\2|')" + declare $var="$val" + case "$var" in + group) _groups="$_groups$group\n" ;; + depend) _depends="$_depends$depend\n" ;; + backup) _backups="$_backups$backup\n" ;; + license) _licenses="$_licenses$license\n" ;; + replaces) _replaces="$_replaces$replaces\n" ;; + provides) _provides="$_provides$provides\n" ;; + conflict) _conflicts="$_conflicts$conflict\n" ;; + optdepend) _optdepends="$_optdepends$optdepend\n" ;; esac done
For testing, I added all the packages in my cache to a database with and without this patch, and the resulting databases are identical so we should be safe. However, we have twice as many forks as before now, so the time went from 1:36 to 3:14 (for adding 493 packages). I don't know if we should care or not, I guess we usually add a small numbers of packages. If we care, we should have a look at all the slowest operations in repo-add.
On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc@gmail.com> wrote:
On Sun, Feb 8, 2009 at 7:31 PM, Dan McGee <dan@archlinux.org> wrote:
Yes, I think I sent this to myself on accident. Looks like it is time for another cup of coffee.
---------- Forwarded message ---------- From: Dan McGee <dan@archlinux.org> Date: Sun, Feb 8, 2009 at 12:30 PM Subject: Re: [PATCH] repo-add: fix eval and quote issues To: Dan McGee <dan@archlinux.org>
eval was ugly and dirty, and bit us here. Instead, use a safer form of variable declaration to ensure quotes don't foil us in pkgdesc or any other fields.
This fixes FS#10837.
Signed-off-by: Dan McGee <dan@archlinux.org> --- We apparently had never handled quotes in descriptions correctly, so
On Sun, Feb 8, 2009 at 12:22 PM, Dan McGee <dan@archlinux.org> wrote: this patch should fix these issues. I'm going to push this patch to maint if there are no objections, although I expect our next release will come from master.
scripts/repo-add.sh.in | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in index be0859e..93fdd52 100644 --- a/scripts/repo-add.sh.in +++ b/scripts/repo-add.sh.in @@ -152,18 +152,19 @@ db_write_entry()
# read info from the zipped package local line I did make one small fix here- added var and val to the local declaration. - for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do - eval "$line" - case "$line" in - group=*) _groups="$_groups$group\n" ;; - depend=*) _depends="$_depends$depend\n" ;; - backup=*) _backups="$_backups$backup\n" ;; - license=*) _licenses="$_licenses$license\n" ;; - replaces=*) _replaces="$_replaces$replaces\n" ;; - provides=*) _provides="$_provides$provides\n" ;; - conflict=*) _conflicts="$_conflicts$conflict\n" ;; - optdepend=*) _optdepends="$_optdepends$optdepend\n" ;; + for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | grep -v '^#'); do + var="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\1|')" + val="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\2|')" + declare $var="$val" + case "$var" in + group) _groups="$_groups$group\n" ;; + depend) _depends="$_depends$depend\n" ;; + backup) _backups="$_backups$backup\n" ;; + license) _licenses="$_licenses$license\n" ;; + replaces) _replaces="$_replaces$replaces\n" ;; + provides) _provides="$_provides$provides\n" ;; + conflict) _conflicts="$_conflicts$conflict\n" ;; + optdepend) _optdepends="$_optdepends$optdepend\n" ;; esac done
For testing, I added all the packages in my cache to a database with and without this patch, and the resulting databases are identical so we should be safe. However, we have twice as many forks as before now, so the time went from 1:36 to 3:14 (for adding 493 packages). I don't know if we should care or not, I guess we usually add a small numbers of packages. If we care, we should have a look at all the slowest operations in repo-add.
The reason I used eval originally was to catch the pkgver=${_somever/-/_} type stuff. Does using declare still work that way? Additionally, regarding what Xavier said: why not keep the loop expression as is and use "declare $line" ?
On Mon, Feb 9, 2009 at 8:28 PM, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
The reason I used eval originally was to catch the pkgver=${_somever/-/_} type stuff. Does using declare still work that way?
Afaik, makepkg sources the PKGBUILD already and creates the .PKGINFO file based on that, so all variables have already been evaluated. I don't see any reason to do it again when repo-add reads the PKGINFO file.
On Mon, Feb 9, 2009 at 1:45 PM, Xavier <shiningxc@gmail.com> wrote:
On Mon, Feb 9, 2009 at 8:28 PM, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
The reason I used eval originally was to catch the pkgver=${_somever/-/_} type stuff. Does using declare still work that way?
Afaik, makepkg sources the PKGBUILD already and creates the .PKGINFO file based on that, so all variables have already been evaluated. I don't see any reason to do it again when repo-add reads the PKGINFO file.
Oh oh, I was thinking something else... pardon me. I guess I didn't realize that we're parsing .PKGINFO files here
Sorry I'm coming into the conversation late (Just joined the listserv). I believe this is about where repo-add does the bsdtar then grep/sed then evals each line. If someone is about to make a change there, could the sed also be changed? Mac OS X's sed doesn't work with \s, so it should be replaced with this: for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do + grep -v "^#" | sed 's|\(\w*\)[ \t]*=[ \t]*\(.*\)|\1="\2"|'); do eval "$line" I have some other patches too that I'll be submitting after I get git setup, so if this is skipped now that's fine. On Mon, Feb 9, 2009 at 5:19 PM, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
On Mon, Feb 9, 2009 at 1:45 PM, Xavier <shiningxc@gmail.com> wrote:
On Mon, Feb 9, 2009 at 8:28 PM, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
The reason I used eval originally was to catch the pkgver=${_somever/-/_} type stuff. Does using declare still work that way?
Afaik, makepkg sources the PKGBUILD already and creates the .PKGINFO file based on that, so all variables have already been evaluated. I don't see any reason to do it again when repo-add reads the PKGINFO file.
Oh oh, I was thinking something else... pardon me. I guess I didn't realize that we're parsing .PKGINFO files here _______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev
Kevin Barry wrote:
Sorry I'm coming into the conversation late (Just joined the listserv). I believe this is about where repo-add does the bsdtar then grep/sed then evals each line. If someone is about to make a change there, could the sed also be changed? Mac OS X's sed doesn't work with \s, so it should be replaced with this:
for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do + grep -v "^#" | sed 's|\(\w*\)[ \t]*=[ \t]*\(.*\)|\1="\2"|'); do eval "$line"
I have some other patches too that I'll be submitting after I get git setup, so if this is skipped now that's fine.
If you have a bunch of Mac OSX compatibility patches, then it would probably be better to address those in the one place and leave this patch doing its fix. Allan
On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc@gmail.com> wrote:
For testing, I added all the packages in my cache to a database with and without this patch, and the resulting databases are identical so we should be safe. However, we have twice as many forks as before now, so the time went from 1:36 to 3:14 (for adding 493 packages). I don't know if we should care or not, I guess we usually add a small numbers of packages. If we care, we should have a look at all the slowest operations in repo-add.
It is actually way worse than 2x the number of forks- we now fork once per file *per line* in our .PKGINFO file, whereas before we only forked once per file. New patch incoming on the ML that should fork as many times as the old version, reducing that bottleneck a lot. -Dan
On Mon, Feb 16, 2009 at 4:17 AM, Dan McGee <dpmcgee@gmail.com> wrote:
On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc@gmail.com> wrote:
For testing, I added all the packages in my cache to a database with and without this patch, and the resulting databases are identical so we should be safe. However, we have twice as many forks as before now, so the time went from 1:36 to 3:14 (for adding 493 packages). I don't know if we should care or not, I guess we usually add a small numbers of packages. If we care, we should have a look at all the slowest operations in repo-add.
It is actually way worse than 2x the number of forks- we now fork once per file *per line* in our .PKGINFO file, whereas before we only forked once per file.
New patch incoming on the ML that should fork as many times as the old version, reducing that bottleneck a lot.
Wow indeed, I always read the old version wrongly, I just realized the old for was split on two lines :
- for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do
On 16/02/2009, at 5:24 PM, Xavier wrote:
On Mon, Feb 16, 2009 at 4:17 AM, Dan McGee <dpmcgee@gmail.com> wrote:
On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc@gmail.com> wrote:
For testing, I added all the packages in my cache to a database with and without this patch, and the resulting databases are identical so we should be safe. However, we have twice as many forks as before now, so the time went from 1:36 to 3:14 (for adding 493 packages). I don't know if we should care or not, I guess we usually add a small numbers of packages. If we care, we should have a look at all the slowest operations in repo-add.
It is actually way worse than 2x the number of forks- we now fork once per file *per line* in our .PKGINFO file, whereas before we only forked once per file.
New patch incoming on the ML that should fork as many times as the old version, reducing that bottleneck a lot.
Wow indeed, I always read the old version wrongly, I just realized the old for was split on two lines :
- for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)| \1="\2"|'); do
How about keeping that part, and just `read`ing the values in: for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1 "\2"|'); do echo $line | read var val declare $var="$val" # Rest is just as in the patch done I haven't actually tested it since the regex is somewhat faulty on Mac OS X for some reason, and I'm too lazy to boot Arch up :P. Note: The substitution now results in "pkgname foo" instead of "pkgname=foo".
On Mon, Feb 16, 2009 at 3:32 AM, Sebastian Nowicki <sebnow@gmail.com> wrote:
On 16/02/2009, at 5:24 PM, Xavier wrote:
On Mon, Feb 16, 2009 at 4:17 AM, Dan McGee <dpmcgee@gmail.com> wrote:
On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc@gmail.com> wrote:
For testing, I added all the packages in my cache to a database with and without this patch, and the resulting databases are identical so we should be safe. However, we have twice as many forks as before now, so the time went from 1:36 to 3:14 (for adding 493 packages). I don't know if we should care or not, I guess we usually add a small numbers of packages. If we care, we should have a look at all the slowest operations in repo-add.
It is actually way worse than 2x the number of forks- we now fork once per file *per line* in our .PKGINFO file, whereas before we only forked once per file.
New patch incoming on the ML that should fork as many times as the old version, reducing that bottleneck a lot.
Wow indeed, I always read the old version wrongly, I just realized the old for was split on two lines :
- for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do
How about keeping that part, and just `read`ing the values in:
for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1 "\2"|'); do echo $line | read var val declare $var="$val" # Rest is just as in the patch done
I haven't actually tested it since the regex is somewhat faulty on Mac OS X for some reason, and I'm too lazy to boot Arch up :P.
When you do test it, I would put my money on it being broken the same way the original one is. :)
Note: The substitution now results in "pkgname foo" instead of "pkgname=foo".
This is more or less what I did in my revised patch, if you notice. However, quote marks in the sed call itself are the whole crux of the issue here, so we shouldn't have them there at all. -Dan
On Mon, Feb 16, 2009 at 2:57 PM, Dan McGee <dpmcgee@gmail.com> wrote:
When you do test it, I would put my money on it being broken the same way the original one is. :)
Note: The substitution now results in "pkgname foo" instead of "pkgname=foo".
This is more or less what I did in my revised patch, if you notice. However, quote marks in the sed call itself are the whole crux of the issue here, so we shouldn't have them there at all.
Is your revised patch out somewhere? I missed it :)
On 16/02/2009, at 10:57 PM, Dan McGee wrote:
On Mon, Feb 16, 2009 at 3:32 AM, Sebastian Nowicki <sebnow@gmail.com> wrote:
On 16/02/2009, at 5:24 PM, Xavier wrote:
On Mon, Feb 16, 2009 at 4:17 AM, Dan McGee <dpmcgee@gmail.com> wrote:
Wow indeed, I always read the old version wrongly, I just realized the old for was split on two lines :
- for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ - grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)| \1="\2"|'); do
How about keeping that part, and just `read`ing the values in:
for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \ grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1 "\2"|'); do echo $line | read var val declare $var="$val" # Rest is just as in the patch done
I haven't actually tested it since the regex is somewhat faulty on Mac OS X for some reason, and I'm too lazy to boot Arch up :P.
When you do test it, I would put my money on it being broken the same way the original one is. :)
*pays up* Indeed, it's broken. I didn't think that the for loop would be the problem. Fortunately this lead to a slightly smaller, albeit less legible, loop instead: $ bsdtar -xOf "$pkgfile" .PKGINFO | grep -v "^#" | sed 's|\(\w*\)\s*= \s*\(.*\)|\1 \2|' | while read var val; do echo declare $var=\"$val\"; done declare pkgname="perl-bit-vector" declare pkgver="6.4-3" declare pkgdesc="Efficient bit vector, set of integers and "big int" math library" declare url="http://search.cpan.org/dist//" declare builddate="1200021098" declare packager="Eric Belanger <eric@archlinux.org>" declare size="463896" declare arch="x86_64" declare license="GPL" declare license="LGPL" declare license="PerlArtistic" declare depend="perl-carp-clan" declare depend="perl>=5.10.0" Seems to work fine on the package in question.
participants (7)
-
Aaron Griffin
-
Allan McRae
-
Dan McGee
-
Dan McGee
-
Kevin Barry
-
Sebastian Nowicki
-
Xavier