[pacman-dev,1/1] libalpm: fix resuming after HTTP error

Message ID 20210602114434.2108474-2-whygowe@gmail.com
State New
Headers show
Series libalpm: fix resuming after HTTP error | expand

Commit Message

Hung-I Wang June 2, 2021, 11:44 a.m. UTC
`curl_retry_next_server` tries to resume a download whenever possible
(introduced in 8bf17b2) even if its preceding HTTP request returns an
error (with status code >= 400, e.g. 404 when a mirror is only partially
synced).

It may result in a corrupted package if the preceding HTTP response with
error carries a non-empty body, which is then written to a tempfile as
if it is part of the package binary.

By activating `CURLOPT_FAILONERROR`, the tempfile won't be written
unless the HTTP response indicates a successful status.

Signed-off-by: Hung-I Wang <whygowe@gmail.com>
---
 lib/libalpm/dload.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Allan McRae June 2, 2021, 12:21 p.m. UTC | #1
On 2/6/21 9:44 pm, Hung-I Wang wrote:
> `curl_retry_next_server` tries to resume a download whenever possible
> (introduced in 8bf17b2) even if its preceding HTTP request returns an
> error (with status code >= 400, e.g. 404 when a mirror is only partially
> synced).
> 
> It may result in a corrupted package if the preceding HTTP response with
> error carries a non-empty body, which is then written to a tempfile as
> if it is part of the package binary.
> 
> By activating `CURLOPT_FAILONERROR`, the tempfile won't be written
> unless the HTTP response indicates a successful status.
> 

Thanks - that option is much more simple than the approaches we had
implemented.

> Signed-off-by: Hung-I Wang <whygowe@gmail.com>
> ---
>  lib/libalpm/dload.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 2d8b4d6d..faa3d9f2 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -338,6 +338,7 @@ static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload)
>  	curl_easy_setopt(curl, CURLOPT_TCP_KEEPINTVL, 60L);
>  	curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
>  	curl_easy_setopt(curl, CURLOPT_PRIVATE, (void *)payload);
> +	curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1);
>  
>  	_alpm_log(handle, ALPM_LOG_DEBUG, "%s: url is %s\n",
>  		payload->remote_name, payload->fileurl);
> @@ -427,7 +428,8 @@ static int curl_retry_next_server(CURLM *curlm, CURL *curl, struct dload_payload
>  	len = strlen(server) + strlen(payload->filepath) + 2;
>  	MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
>  	snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> -
> +	_alpm_log(handle, ALPM_LOG_DEBUG, "%s: url is %s\n",
> +		payload->remote_name, payload->fileurl);
>  
>  	fflush(payload->localf);
>  
> @@ -496,6 +498,7 @@ static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
>  	/* was it a success? */
>  	switch(curlerr) {
>  		case CURLE_OK:

Can break here

> +		case CURLE_HTTP_RETURNED_ERROR:
>  			/* get http/ftp response code */
>  			_alpm_log(handle, ALPM_LOG_DEBUG, "%s: response code %ld\n",
>  					payload->remote_name, payload->respcode);

and here we would no longer need to check for respcode>=400
Andrew Gregory June 2, 2021, 2:07 p.m. UTC | #2
On 06/02/21 at 07:44pm, Hung-I Wang wrote:
> `curl_retry_next_server` tries to resume a download whenever possible
> (introduced in 8bf17b2) even if its preceding HTTP request returns an
> error (with status code >= 400, e.g. 404 when a mirror is only partially
> synced).
> 
> It may result in a corrupted package if the preceding HTTP response with
> error carries a non-empty body, which is then written to a tempfile as
> if it is part of the package binary.
> 
> By activating `CURLOPT_FAILONERROR`, the tempfile won't be written
> unless the HTTP response indicates a successful status.
> 
> Signed-off-by: Hung-I Wang <whygowe@gmail.com>
> ---
>  lib/libalpm/dload.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

See 7a5e41925f72d838eaa611427e5ae89b1f57215f:

    dload: avoid using CURLOPT_FAILONERROR
    
    Use of this flag causes connections to be closed on 404s -- a common
    occurrence when your config sets DatabaseOptional. Handle the error
    gracefully, so that the connection can be reused.
    
    Signed-off-by: Dave Reisner <dreisner@archlinux.org>
    Signed-off-by: Allan McRae <allan@archlinux.org>
Allan McRae June 2, 2021, 11:29 p.m. UTC | #3
On 3/6/21 12:07 am, Andrew Gregory wrote:
> On 06/02/21 at 07:44pm, Hung-I Wang wrote:
>> `curl_retry_next_server` tries to resume a download whenever possible
>> (introduced in 8bf17b2) even if its preceding HTTP request returns an
>> error (with status code >= 400, e.g. 404 when a mirror is only partially
>> synced).
>>
>> It may result in a corrupted package if the preceding HTTP response with
>> error carries a non-empty body, which is then written to a tempfile as
>> if it is part of the package binary.
>>
>> By activating `CURLOPT_FAILONERROR`, the tempfile won't be written
>> unless the HTTP response indicates a successful status.
>>
>> Signed-off-by: Hung-I Wang <whygowe@gmail.com>
>> ---
>>  lib/libalpm/dload.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> See 7a5e41925f72d838eaa611427e5ae89b1f57215f:
> 
>     dload: avoid using CURLOPT_FAILONERROR
>     
>     Use of this flag causes connections to be closed on 404s -- a common
>     occurrence when your config sets DatabaseOptional. Handle the error
>     gracefully, so that the connection can be reused.
>     
>     Signed-off-by: Dave Reisner <dreisner@archlinux.org>
>     Signed-off-by: Allan McRae <allan@archlinux.org>
> 

Ah - thanks.   Will stick with the patch I submitted then.

Allan

Patch

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 2d8b4d6d..faa3d9f2 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -338,6 +338,7 @@  static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload)
 	curl_easy_setopt(curl, CURLOPT_TCP_KEEPINTVL, 60L);
 	curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 	curl_easy_setopt(curl, CURLOPT_PRIVATE, (void *)payload);
+	curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1);
 
 	_alpm_log(handle, ALPM_LOG_DEBUG, "%s: url is %s\n",
 		payload->remote_name, payload->fileurl);
@@ -427,7 +428,8 @@  static int curl_retry_next_server(CURLM *curlm, CURL *curl, struct dload_payload
 	len = strlen(server) + strlen(payload->filepath) + 2;
 	MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
 	snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
-
+	_alpm_log(handle, ALPM_LOG_DEBUG, "%s: url is %s\n",
+		payload->remote_name, payload->fileurl);
 
 	fflush(payload->localf);
 
@@ -496,6 +498,7 @@  static int curl_check_finished_download(CURLM *curlm, CURLMsg *msg,
 	/* was it a success? */
 	switch(curlerr) {
 		case CURLE_OK:
+		case CURLE_HTTP_RETURNED_ERROR:
 			/* get http/ftp response code */
 			_alpm_log(handle, ALPM_LOG_DEBUG, "%s: response code %ld\n",
 					payload->remote_name, payload->respcode);