[pacman-dev] [PATCH 1/2] dload: unhook error buffer after transfer finishes
Similar to what we did in edd9ed6a, disconnect the relationship with our stack allocated error buffer from the curl handle. Just as an FTP connection might have some network chatter on teardown causing the progress callback to be triggered, we might also hit an error condition that causes curl to write to our (now out of scope) error buffer. I'm unable to reproduce FS#26327, but I have a suspicion that this should fix it. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 33824be..e848b30 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -377,8 +377,11 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl); - /* immediately unhook the progress callback */ + /* disconnect relationships from the curl handle for things that might go out + * of scope, but could still be touched on connection teardown. This really + * only applies to FTP transfers. See FS#26327 for an example. */ curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, (char *)0); /* was it a success? */ switch(payload->curlerr) { -- 1.7.7
This doesn't actually change the treatment of install and changelog varaibles, but it allows valid bash syntax to get by our parsing of these variables. Fixes FS#25827. Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- This is getting pretty ugly... I'm still 50/50 on whether or not we should do this. I will point out that if we choose not to merge this, there's a dozen or so packages in extra/community that need to be fixed: $ grep -lE '^(install|changelog)=\(' /var/abs/*/*/PKGBUILD And just because I'm getting a little paranoid about making changes this close to release, here's the test rig I used to proof this patch: http://paste.xinu.at/XcbR/ Allan, thoughts? scripts/makepkg.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 84221d0..f2ad16d 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1537,7 +1537,7 @@ check_sanity() { local file while read -r file; do # evaluate any bash variables used - eval file=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/' <<< "$file")\" + eval file=\"$(sed 's/^(\(.*\))$/\1/;s/^\(['\''"]\)\(.*\)\1$/\2/' <<< "$file")\" if [[ $file && ! -f $file ]]; then error "$(gettext "%s file (%s) does not exist.")" "$i" "$file" ret=1 -- 1.7.7
On 10/10/11 13:35, Dave Reisner wrote:
This doesn't actually change the treatment of install and changelog varaibles, but it allows valid bash syntax to get by our parsing of these variables.
Fixes FS#25827.
Signed-off-by: Dave Reisner<dreisner@archlinux.org> --- This is getting pretty ugly... I'm still 50/50 on whether or not we should do this. I will point out that if we choose not to merge this, there's a dozen or so packages in extra/community that need to be fixed:
$ grep -lE '^(install|changelog)=\(' /var/abs/*/*/PKGBUILD
And just because I'm getting a little paranoid about making changes this close to release, here's the test rig I used to proof this patch:
Allan, thoughts?
I will give this an ack. I lean ever so slightly to the side that if bash accepts that syntax then so should we. So given this is a fairly small fix, I see no point in not including it. I added a couple more cases to your tests (involving files with spaces...) and all seems good to me. Allan
On Mon, Oct 10, 2011 at 01:53:38PM +1000, Allan McRae wrote:
On 10/10/11 13:35, Dave Reisner wrote:
This doesn't actually change the treatment of install and changelog varaibles, but it allows valid bash syntax to get by our parsing of these variables.
Fixes FS#25827.
Signed-off-by: Dave Reisner<dreisner@archlinux.org> --- This is getting pretty ugly... I'm still 50/50 on whether or not we should do this. I will point out that if we choose not to merge this, there's a dozen or so packages in extra/community that need to be fixed:
$ grep -lE '^(install|changelog)=\(' /var/abs/*/*/PKGBUILD
And just because I'm getting a little paranoid about making changes this close to release, here's the test rig I used to proof this patch:
Allan, thoughts?
I will give this an ack. I lean ever so slightly to the side that if bash accepts that syntax then so should we. So given this is a fairly small fix, I see no point in not including it.
I added a couple more cases to your tests (involving files with spaces...) and all seems good to me.
Allan
Ok -- pushing a slightly revised version to my working branch then which uses -e 's/ex/pr/' -e 's/ex/pr/' because I'm suddenly reminded that combining expressions isn't supported with BSD sed. d
On Sun, Oct 9, 2011 at 10:35 PM, Dave Reisner <d@falconindy.com> wrote:
Similar to what we did in edd9ed6a, disconnect the relationship with our stack allocated error buffer from the curl handle. Just as an FTP connection might have some network chatter on teardown causing the progress callback to be triggered, we might also hit an error condition that causes curl to write to our (now out of scope) error buffer.
I'm unable to reproduce FS#26327, but I have a suspicion that this should fix it.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 33824be..e848b30 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -377,8 +377,11 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl);
- /* immediately unhook the progress callback */ + /* disconnect relationships from the curl handle for things that might go out + * of scope, but could still be touched on connection teardown. This really + * only applies to FTP transfers. See FS#26327 for an example. */ curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, (char *)0); 0, aka NULL? Don't mix integers and pointers please, and the cast thus becomes unnecessary.
/* was it a success? */ switch(payload->curlerr) { -- 1.7.7
On Sun, Oct 09, 2011 at 10:50:08PM -0500, Dan McGee wrote:
On Sun, Oct 9, 2011 at 10:35 PM, Dave Reisner <d@falconindy.com> wrote:
Similar to what we did in edd9ed6a, disconnect the relationship with our stack allocated error buffer from the curl handle. Just as an FTP connection might have some network chatter on teardown causing the progress callback to be triggered, we might also hit an error condition that causes curl to write to our (now out of scope) error buffer.
I'm unable to reproduce FS#26327, but I have a suspicion that this should fix it.
Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/dload.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 33824be..e848b30 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -377,8 +377,11 @@ static int curl_download_internal(struct dload_payload *payload, /* perform transfer */ payload->curlerr = curl_easy_perform(curl);
- /* immediately unhook the progress callback */ + /* disconnect relationships from the curl handle for things that might go out + * of scope, but could still be touched on connection teardown. This really + * only applies to FTP transfers. See FS#26327 for an example. */ curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 1L); + curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, (char *)0); 0, aka NULL? Don't mix integers and pointers please, and the cast thus becomes unnecessary.
Sorry, should have mentioned that this won't compile using NULL because of the typechecking curl does: dload.c:384:2: error: call to '_curl_easy_setopt_err_error_buffer' declared with attribute warning: curl_easy_setopt expects a char buffer of CURL_ERROR_SIZE as argument for this option [-Werror] d
/* was it a success? */ switch(payload->curlerr) { -- 1.7.7
participants (3)
-
Allan McRae
-
Dan McGee
-
Dave Reisner