[pacman-dev] [PATCH] Handle bad .part files when encountered
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@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; + 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; + } } /* tell the caller that we have a partial */ -- 2.7.4
Heyho Just two suggestions below. On Fri, Apr 15, 2016 at 1:54 PM, Allan McRae <allan@archlinux.org> 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@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; + goto finish; + } else if (size == 0) { + /* the .part file is complete, but needs renamed */
This comment should probably be: /* the .part file is complete, but needs to be 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';
Maybe newpath[fpathlen - strlen(".part")] = '\0'; would be clearer? Cheers, Silvan
+ rename(fpath, newpath); + FREE(newpath); + + goto finish; + } }
/* tell the caller that we have a partial */ -- 2.7.4
On 15/04/16 22:10, Silvan Jegen wrote:
Heyho
Just two suggestions below.
On Fri, Apr 15, 2016 at 1:54 PM, Allan McRae <allan@archlinux.org> 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@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; + goto finish; + } else if (size == 0) { + /* the .part file is complete, but needs renamed */
This comment should probably be:
/* the .part file is complete, but needs to be 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';
Maybe
newpath[fpathlen - strlen(".part")] = '\0';
would be clearer?
Both adjusted. Thanks for the review! A
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@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
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Silvan Jegen