[pacman-dev] [PATCH] Handle bad .part files when encountered

[Andrew Gregory]

On 04/15/16 at 09:54pm, Allan McRae wrote:
> We can end up with .part files downloaded that are unable to be used by pacman
> in two ways:  1) a well timed ctrl+c right at the rename of the download file
> can leave a complete download with a .part extension, or 2) for some reason
> (bad mirror) a .part file can be larger than the file to be downloaded. In
> both cases, pacman fails with a very cryptic error message.
> 
> When these files are encountered by libalpm, they are now either renamed to
> remove the if .part suffix if the download is complete, or deleted if the
> download file is too large.
> 
> Fixes FS#35789 and FS#39257.
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---
>  lib/libalpm/sync.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 00b68d0..95e34f8 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -337,7 +337,27 @@ static int compute_download_size(alpm_pkg_t *newpkg)
>  			/* subtract the size of the .part file */
>  			_alpm_log(handle, ALPM_LOG_DEBUG, "using (package - .part) size\n");
>  			size = newpkg->size - st.st_size;
> -			size = size < 0 ? 0 : size;
> +			if(size < 0) {
> +				/* we have a bad .part file - delete it */
> +				_alpm_log(handle, ALPM_LOG_DEBUG, "deleting invalid .part file\n");
> +				unlink(fpath);
> +				size = newpkg->size;

This is true, but only because our download code is broken elsewhere.
Even though we count them here, .part files in any non-primary cache
directories are completely ignored.  We only ever download to our
first writable cache even if there are partial downloads in a later
cache.

> +				goto finish;
> +			} else if (size == 0) {
> +				/* the .part file is complete, but needs renamed */
> +				char *newpath;
> +				size_t fpathlen = strlen(fpath);
> +
> +				_alpm_log(handle, ALPM_LOG_DEBUG, "renaming complete .part file\n");
> +
> +				CALLOC(newpath, fpathlen + 1, sizeof(char), return -1);
> +				strcpy(newpath, fpath);
> +				newpath[fpathlen - 5] = '\0';
> +				rename(fpath, newpath);
> +				FREE(newpath);
> +
> +				goto finish;
> +			}

unlink and rename both need to be checked for success, otherwise the
user can still get the cryptic error message.

I don't really like having this in this function.  This is a query
function that's used any time alpm_pkg_download_size is called.
I don't like having query functions make, or at least try to make,
modifications to the filesystem.

>  		}
>  
>  		/* tell the caller that we have a partial */
> -- 
> 2.7.4