Return with ALPM_ERR_WRONG_ARGS instead of causing a potential segfault if alpm_fetch_pkgurl() is invoked with a NULL URL. Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- lib/libalpm/dload.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index a1b6364..0e1c64b 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -450,6 +450,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url) int ret; CHECK_HANDLE(handle, return NULL); + ASSERT(url, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL)); /* find a valid cache dir to download to */ cachedir = _alpm_filecache_setup(handle); -- 1.7.6

On Thu, Aug 18, 2011 at 2:42 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote:

Use the STRDUP macro instead of strdup() for the sake of better error handling on memory allocation failures.

Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de>

Thanks- wrapped at 80 chars though.

--- Should we do some cleanup and free() payload if STRDUP() fails in alpm_fetch_pkgurl()?

Even in this case, I'm not sure the immediate RET_ERR() is the best thing to do. Instead you should probably set pm_errno and make sure ret = -1, but not return right away so that the other cleanup logic takes place (notably the signal handler restoration, didn't realize that was so late). In fetch_pkgurl(), you may want to add an error: label that is jumped to instead of the RET_ERR() calls directly, and add the necessary cleanup logic there, or something along those lines to ensure things are unwound correctly. However, for the most part, we assume a memory allocation failure is not something transient and we will eventually die.

 lib/libalpm/dload.c |    6 +++---  1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 68a68e9..a1b6364 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -387,10 +387,10 @@ cleanup:                                                tempfile, destfile, strerror(errno));                                ret = -1;                        } else if(final_file) { -                               *final_file = strdup(strrchr(destfile, '/') + 1); +                               STRDUP(*final_file, strrchr(destfile, '/') + 1, RET_ERR(handle, ALPM_ERR_MEMORY, -1));                        }                } else if(final_file) { -                       *final_file = strdup(strrchr(tempfile, '/') + 1); +                       STRDUP(*final_file, strrchr(tempfile, '/') + 1, RET_ERR(handle, ALPM_ERR_MEMORY, -1));                }        }

@@ -456,7 +456,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url)

       CALLOC(payload, 1, sizeof(*payload), RET_ERR(handle, ALPM_ERR_MEMORY, NULL));        payload->handle = handle; -       payload->fileurl = strdup(url); +       STRDUP(payload->fileurl, url, RET_ERR(handle, ALPM_ERR_MEMORY, NULL));        payload->allow_resume = 1;

       /* download the file */ -- 1.7.6