[pacman-dev] [PATCH 3/4] lib/dload: major refactor of signature downloading

Dave Reisner d at falconindy.com
Mon Mar 28 15:15:42 EDT 2011


There's a lot of related moving parts here:
* iteration through mirrors is moved back to the calling functions. this
  allows removal of _alpm_download_single_file and _alpm_download_files
* rename download => _alpm_download. also modified to accept an extra
  arg of type pgp_verify_t which is passed through to
  curl_download_internal
* move the actual signature download to curl_download_internal. this
  allows us to ensure that the signature and the file came from the
  same mirror, and only to accept the file if signature verification
  passes.

Signed-off-by: Dave Reisner <d at falconindy.com>
---
 lib/libalpm/be_sync.c |   71 ++++++----------------
 lib/libalpm/dload.c   |  159 +++++++++++++++++++++++++++++--------------------
 lib/libalpm/dload.h   |    8 +--
 lib/libalpm/sync.c    |   27 +++++++-
 4 files changed, 139 insertions(+), 126 deletions(-)

diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
index 756f784..ce62c2b 100644
--- a/lib/libalpm/be_sync.c
+++ b/lib/libalpm/be_sync.c
@@ -79,11 +79,12 @@
  */
 int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
 {
-	char *dbfile, *syncpath;
+	char *syncpath;
 	const char *dbpath;
+	alpm_list_t *i;
 	struct stat buf;
 	size_t len;
-	int ret;
+	int ret = -1;
 	mode_t oldmask;
 
 	ALPM_LOG_FUNC;
@@ -91,14 +92,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
 	/* Sanity checks */
 	ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
 	ASSERT(db != NULL && db != handle->db_local, RET_ERR(PM_ERR_WRONG_ARGS, -1));
-
-	if(!alpm_list_find_ptr(handle->dbs_sync, db)) {
-		RET_ERR(PM_ERR_DB_NOT_FOUND, -1);
-	}
-
-	len = strlen(db->treename) + 4;
-	MALLOC(dbfile, len, RET_ERR(PM_ERR_MEMORY, -1));
-	sprintf(dbfile, "%s.db", db->treename);
+	ASSERT(db->servers != NULL, RET_ERR(PM_ERR_SERVER_NONE, -1));
 
 	dbpath = alpm_option_get_dbpath();
 	len = strlen(dbpath) + 6;
@@ -112,20 +106,33 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
 		_alpm_log(PM_LOG_DEBUG, "database dir '%s' does not exist, creating it\n",
 				syncpath);
 		if(_alpm_makepath(syncpath) != 0) {
-			free(dbfile);
 			free(syncpath);
 			RET_ERR(PM_ERR_SYSTEM, -1);
 		}
 	} else if(!S_ISDIR(buf.st_mode)) {
 		_alpm_log(PM_LOG_WARNING, _("removing invalid file: %s\n"), syncpath);
 		if(unlink(syncpath) != 0 || _alpm_makepath(syncpath) != 0) {
-			free(dbfile);
 			free(syncpath);
 			RET_ERR(PM_ERR_SYSTEM, -1);
 		}
 	}
 
-	ret = _alpm_download_single_file(dbfile, db->servers, syncpath, force);
+	for(i = db->servers; i; i = i->next) {
+		const char *server = i->data;
+		char *fileurl;
+		size_t len;
+
+		/* print server + filename into a buffer */
+		len = strlen(server) + strlen(db->treename) + 5;
+		CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1));
+		snprintf(fileurl, len, "%s/%s.db", server, db->treename);
+
+		ret = _alpm_download(fileurl, syncpath, force, db->pgp_verify);
+		FREE(fileurl);
+		if(ret != -1) {
+			break;
+		}
+	}
 
 	if(ret == 1) {
 		/* files match, do nothing */
@@ -137,49 +144,11 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db)
 		goto cleanup;
 	}
 
-	/* Download and check the signature of the database if needed */
-	if(db->pgp_verify != PM_PGP_VERIFY_NEVER) {
-		char *sigfile, *sigfilepath;
-		int sigret;
-
-		len = strlen(dbfile) + 5;
-		MALLOC(sigfile, len, RET_ERR(PM_ERR_MEMORY, -1));
-		sprintf(sigfile, "%s.sig", dbfile);
-
-		/* prevent old signature being used if the following download fails */
-		len = strlen(syncpath) + strlen(sigfile) + 1;
-		MALLOC(sigfilepath, len, RET_ERR(PM_ERR_MEMORY, -1));
-		sprintf(sigfilepath, "%s%s", syncpath, sigfile);
-		_alpm_rmrf(sigfilepath);
-		free(sigfilepath);
-
-		sigret = _alpm_download_single_file(sigfile, db->servers, syncpath, 0);
-		free(sigfile);
-
-		if(sigret == -1 && db->pgp_verify == PM_PGP_VERIFY_ALWAYS) {
-			_alpm_log(PM_LOG_ERROR, _("Failed to download signature for db: %s\n"),
-					alpm_strerrorlast());
-			pm_errno = PM_ERR_SIG_INVALID;
-			ret = -1;
-			goto cleanup;
-		}
-
-		sigret = alpm_db_check_pgp_signature(db);
-		if((db->pgp_verify == PM_PGP_VERIFY_ALWAYS && sigret != 0) ||
-				(db->pgp_verify == PM_PGP_VERIFY_OPTIONAL && sigret == 1)) {
-			/* pm_errno was set by the checking code */
-			/* TODO: should we just leave the unverified database */
-			ret = -1;
-			goto cleanup;
-		}
-	}
-
 	/* Cache needs to be rebuilt */
 	_alpm_db_free_pkgcache(db);
 
 cleanup:
 
-	free(dbfile);
 	free(syncpath);
 	umask(oldmask);
 	return ret;
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
index 948e623..69fc708 100644
--- a/lib/libalpm/dload.c
+++ b/lib/libalpm/dload.c
@@ -155,15 +155,15 @@ static int utimes_long(const char *path, long time)
 
 
 static int curl_download_internal(const char *url, const char *localpath,
-		int force)
+		int force, pgp_verify_t check_sig)
 {
-	int ret = -1;
+	int ret = -1, sig_ret = -1;
 	FILE *localf = NULL;
 	const char *open_mode, *useragent;
 	char *destfile, *tempfile;
 	char hostname[256]; /* RFC1123 states applications should support this length */
 	struct stat st;
-	long httpresp, timecond, remote_time;
+	long resp_code, timecond, remote_time;
 	double remote_size, bytes_dl;
 	struct sigaction sig_pipe[2], sig_int[2];
 	struct fileinfo dlfile;
@@ -242,7 +242,7 @@ static int curl_download_internal(const char *url, const char *localpath,
 	handle->curlerr = curl_easy_perform(handle->curl);
 
 	/* retrieve info about the state of the transfer */
-	curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &httpresp);
+	curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &resp_code);
 	curl_easy_getinfo(handle->curl, CURLINFO_FILETIME, &remote_time);
 	curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size);
 	curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl);
@@ -279,15 +279,92 @@ static int curl_download_internal(const char *url, const char *localpath,
 
 	ret = 0;
 
+	/* we were successful downloading the file. now we need to fetch a signature
+	 * if necessary. we already know that a new file was actually fetched; the
+	 * not modified case was handled and we jumped to label cleanup already. */
+	if(check_sig != PM_PGP_VERIFY_NEVER) {
+		FILE *sigf = NULL;
+		char *sig_url, *sig_temp = NULL, *sig_dest = NULL;
+		size_t len;
+		CURLcode sig_curlerr;
+
+		/* if we enter this block, assume failure unless we explicitly state
+		 * otherwise */
+		sig_ret = -1;
+
+		len = strlen(url) + 5;
+		CALLOC(sig_url, len, sizeof(char), goto sig_cleanup);
+		snprintf(sig_url, len, "%s.sig", url);
+
+		dlfile.filename = get_filename(sig_url);
+		dlfile.initial_size = 0;
+
+		sig_dest = get_fullpath(localpath, dlfile.filename, "");
+		sig_temp = get_fullpath(localpath, dlfile.filename, ".part");
+
+		curl_easy_setopt(handle->curl, CURLOPT_URL, sig_url);
+		curl_easy_setopt(handle->curl, CURLOPT_PROGRESSDATA, (void *)&dlfile);
+		curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, 0L);
+		curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, 0L);
+		curl_easy_setopt(handle->curl, CURLOPT_RESUME_FROM, 0L);
+
+		sigf = fopen(sig_temp, "wb");
+		if(sigf == NULL) {
+			goto sig_cleanup;
+		}
+		curl_easy_setopt(handle->curl, CURLOPT_WRITEDATA, sigf);
+
+		/* Progress 0 - initialize */
+		prevprogress = 0;
+
+		/* perform transfer */
+		sig_curlerr = curl_easy_perform(handle->curl);
+
+		/* retrieve info about the state of the transfer */
+		curl_easy_getinfo(handle->curl, CURLINFO_RESPONSE_CODE, &resp_code);
+		curl_easy_getinfo(handle->curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &remote_size);
+		curl_easy_getinfo(handle->curl, CURLINFO_SIZE_DOWNLOAD, &bytes_dl);
+
+		if(sig_curlerr == CURLE_ABORTED_BY_CALLBACK) {
+			goto sig_cleanup;
+		} else if((sig_curlerr == CURLE_REMOTE_FILE_NOT_FOUND
+					|| sig_curlerr == CURLE_FTP_COULDNT_RETR_FILE
+					|| (sig_curlerr == CURLE_HTTP_RETURNED_ERROR && resp_code == 404))
+				&& check_sig != PM_PGP_VERIFY_ALWAYS) {
+			/* an error case that is not fatal */
+			sig_ret = 1;
+			goto sig_cleanup;
+		} else if(sig_curlerr != CURLE_OK) {
+			pm_errno = PM_ERR_LIBCURL;
+			handle->curlerr = sig_curlerr;
+			_alpm_log(PM_LOG_ERROR, _("failed retrieving file '%s' from %s : %s\n"),
+					dlfile.filename, hostname, curl_easy_strerror(handle->curlerr));
+			/* unlink both files- they are only good if they both were downloaded */
+			unlink(sig_temp);
+			unlink(tempfile);
+			goto sig_cleanup;
+		}
+
+		sig_ret = 0;
+
+sig_cleanup:
+		if(sigf) {
+			fclose(sigf);
+		}
+		if(sig_ret == 0) {
+			rename(sig_temp, sig_dest);
+		}
+		FREE(sig_temp);
+		FREE(sig_dest);
+	}
+
 cleanup:
 	if(localf != NULL) {
 		fclose(localf);
 		utimes_long(tempfile, remote_time);
 	}
 
-	/* TODO: A signature download will need to return success here as well before
-	 * we're willing to rotate the new file into place. */
-	if(ret == 0) {
+	if(ret == 0 && sig_ret != 0) {
 		rename(tempfile, destfile);
 	}
 
@@ -306,12 +383,12 @@ cleanup:
 }
 #endif
 
-static int download(const char *url, const char *localpath,
-		int force)
+int _alpm_download(const char *url, const char *localpath,
+		int force, pgp_verify_t check_sig)
 {
 	if(handle->fetchcb == NULL) {
 #ifdef HAVE_LIBCURL
-		return curl_download_internal(url, localpath, force);
+		return curl_download_internal(url, localpath, force, check_sig);
 #else
 		RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1);
 #endif
@@ -320,62 +397,14 @@ static int download(const char *url, const char *localpath,
 		if(ret == -1) {
 			RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1);
 		}
-		return ret;
-	}
-}
-
-/*
- * Download a single file
- *   - servers must be a list of urls WITHOUT trailing slashes.
- *
- * RETURN:  0 for successful download
- *          1 if the files are identical
- *         -1 on error
- */
-int _alpm_download_single_file(const char *filename,
-		alpm_list_t *servers, const char *localpath,
-		int force)
-{
-	alpm_list_t *i;
-	int ret = -1;
-
-	ASSERT(servers != NULL, RET_ERR(PM_ERR_SERVER_NONE, -1));
-
-	for(i = servers; i; i = i->next) {
-		const char *server = i->data;
-		char *fileurl = NULL;
-		size_t len;
-
-		/* print server + filename into a buffer */
-		len = strlen(server) + strlen(filename) + 2;
-		CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1));
-		snprintf(fileurl, len, "%s/%s", server, filename);
-
-		ret = download(fileurl, localpath, force);
-		FREE(fileurl);
-		if(ret != -1) {
-			break;
-		}
-	}
-
-	return ret;
-}
-
-int _alpm_download_files(alpm_list_t *files,
-		alpm_list_t *servers, const char *localpath)
-{
-	int ret = 0;
-	alpm_list_t *lp;
-
-	for(lp = files; lp; lp = lp->next) {
-		char *filename = lp->data;
-		if(_alpm_download_single_file(filename, servers,
-					localpath, 0) == -1) {
-			ret++;
+		if(check_sig != PM_PGP_VERIFY_NEVER) {
+			int ret = handle->fetchcb(url, localpath, force);
+			if(ret == -1 && check_sig == PM_PGP_VERIFY_ALWAYS) {
+				RET_ERR(PM_ERR_EXTERNAL_DOWNLOAD, -1);
+			}
 		}
+		return ret;
 	}
-
-	return ret;
 }
 
 /** Fetch a remote pkg.
@@ -397,7 +426,7 @@ char SYMEXPORT *alpm_fetch_pkgurl(const char *url)
 	cachedir = _alpm_filecache_setup();
 
 	/* download the file */
-	ret = download(url, cachedir, 0);
+	ret = _alpm_download(url, cachedir, 0, 1);
 	if(ret == -1) {
 		_alpm_log(PM_LOG_WARNING, _("failed to download %s\n"), url);
 		return NULL;
diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h
index 5ce44b8..60588ee 100644
--- a/lib/libalpm/dload.h
+++ b/lib/libalpm/dload.h
@@ -31,12 +31,8 @@ struct fileinfo {
 	double initial_size;
 };
 
-int _alpm_download_single_file(const char *filename,
-		alpm_list_t *servers, const char *localpath,
-		int force);
-
-int _alpm_download_files(alpm_list_t *files,
-		alpm_list_t *servers, const char *localpath);
+int _alpm_download(const char *url, const char *localpath,
+		int force, pgp_verify_t check_sig);
 
 #endif /* _ALPM_DLOAD_H */
 
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
index 5428e40..bd4d8e9 100644
--- a/lib/libalpm/sync.c
+++ b/lib/libalpm/sync.c
@@ -689,8 +689,8 @@ static int test_md5sum(pmtrans_t *trans, const char *filepath,
 
 int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data)
 {
-	alpm_list_t *i, *j, *files = NULL;
-	alpm_list_t *deltas = NULL;
+	alpm_list_t *i, *j, *k;
+	alpm_list_t *files = NULL, *deltas = NULL;
 	size_t numtargs, current = 0, replaces = 0;
 	int errors = 0;
 	const char *cachedir = NULL;
@@ -758,9 +758,28 @@ int _alpm_sync_commit(pmtrans_t *trans, pmdb_t *db_local, alpm_list_t **data)
 
 		if(files) {
 			EVENT(trans, PM_TRANS_EVT_RETRIEVE_START, current->treename, NULL);
-			errors = _alpm_download_files(files, current->servers, cachedir);
+			for(j = files; j; j = j->next) {
+				const char *filename = j->data;
+				for(k = current->servers; k; k = k->next) {
+					const char *server = k->data;
+					char *fileurl;
+					size_t len;
+
+					/* print server + filename into a buffer */
+					len = strlen(server) + strlen(filename) + 2;
+					CALLOC(fileurl, len, sizeof(char), RET_ERR(PM_ERR_MEMORY, -1));
+					snprintf(fileurl, len, "%s/%s", server, filename);
+
+					ret = _alpm_download(fileurl, cachedir, 0, 0);
+					FREE(fileurl);
+					if(ret != -1) {
+						break;
+					}
+					errors++;
+				}
+			}
 
-			if (errors) {
+			if(errors) {
 				_alpm_log(PM_LOG_WARNING, _("failed to retrieve some files from %s\n"),
 						current->treename);
 				if(pm_errno == 0) {
-- 
1.7.4.2



More information about the pacman-dev mailing list