[pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB
As seen: https://bbs.archlinux.org/viewtopic.php?pid=1297766 Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- lib/libalpm/be_sync.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index feda6f5..f3e0a33 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -561,6 +561,13 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, return -1; } + if(filename == NULL) { + /* A file exists outside of a subdirectory. This isn't a read error, so return + * success and try to continue on. */ + _alpm_log(db->handle, ALPM_LOG_DEBUG, "unknown database file: %s\n", filename); + return 0; + } + if(strcmp(filename, "desc") == 0 || strcmp(filename, "depends") == 0 || (strcmp(filename, "deltas") == 0 && db->handle->deltaratio > 0.0) ) { int ret; -- 1.8.3.2
On 08/07/13 09:53, Dave Reisner wrote:
As seen: https://bbs.archlinux.org/viewtopic.php?pid=1297766
I know it takes some time, but actually explaining what was seen in that thread rather than just linking it will save everyone who looks at this commit some time.
Only a debug level statement? I think a warning would be appropriate: warning: database "foo" contains unknown file: ... But need to check where that would actually print.
On Mon, Jul 08, 2013 at 11:22:15AM +1000, Allan McRae wrote:
Fair enough... and who knows, maybe the bbs will go away some day ;)
I copied this from the final else branch of the if/else tree in sync_db_read() which relegates messages about unknown entries to debug level as well. This was strictly for consistency -- if you'd prefer that we print both of these as warnings, I can make that change as part of this patch.
If a sync DB is malformed and contains entries in the root of the archive, load_pkg_for_entry will leave the 'filename' variable empty, leading to a crash in the ensuing strcmp() calls which determine the DB fragment being examined. While this isn't a read error, this should be reported to the user so that it can be addressed by the creator of the DB. This change promotes the same pre-existing debug statement to a warning as well so that all DB abnormalities are surfaced by the frontend. As seen: https://bbs.archlinux.org/viewtopic.php?pid=1297766 Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- v2: promote debug statements to warnings, add gettext wrapper lib/libalpm/be_sync.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index feda6f5..21e36fe 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -561,6 +561,14 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, return -1; } + if(filename == NULL) { + /* A file exists outside of a subdirectory. This isn't a read error, so return + * success and try to continue on. */ + _alpm_log(db->handle, ALPM_LOG_WARNING, _("unknown database file: %s\n"), + filename); + return 0; + } + if(strcmp(filename, "desc") == 0 || strcmp(filename, "depends") == 0 || (strcmp(filename, "deltas") == 0 && db->handle->deltaratio > 0.0) ) { int ret; @@ -657,7 +665,8 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, /* currently do nothing with this file */ } else { /* unknown database file */ - _alpm_log(db->handle, ALPM_LOG_DEBUG, "unknown database file: %s\n", filename); + _alpm_log(db->handle, ALPM_LOG_WARNING, _("unknown database file: %s\n"), + filename); } return 0; -- 1.8.3.2
On Sun, Jul 7, 2013 at 8:39 PM, Dave Reisner <d@falconindy.com> wrote:
I think we made this a debug message to prevent it from being printed waaaaaay too much- aka once per directory if for some reason we added a few file to sync databases that older pacman versions didn't know about. It is probably OK to print the new one at warning level, but I wouldn't mess with the older message. -Dan
On Mon, Jul 08, 2013 at 09:52:31AM -0500, Dan McGee wrote:
Aha, good point.
It is probably OK to print the new one at warning level, but I wouldn't mess with the older message.
Makes sense. I'll send a v3 which should be a nice compromise between v1 and v2. d
If a sync DB is malformed and contains entries in the root of the archive, load_pkg_for_entry will leave the 'filename' variable empty, leading to a crash in the ensuing strcmp() calls which determine the DB fragment being examined. While this isn't a read error, this should be reported to the user so that it can be addressed by the creator of the DB. As seen: https://bbs.archlinux.org/viewtopic.php?pid=1297766 Signed-off-by: Dave Reisner <dreisner@archlinux.org> --- v3: ignore any changes to the existing catchall. lib/libalpm/be_sync.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index feda6f5..5cc3de3 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -561,6 +561,14 @@ static int sync_db_read(alpm_db_t *db, struct archive *archive, return -1; } + if(filename == NULL) { + /* A file exists outside of a subdirectory. This isn't a read error, so return + * success and try to continue on. */ + _alpm_log(db->handle, ALPM_LOG_WARNING, _("unknown database file: %s\n"), + filename); + return 0; + } + if(strcmp(filename, "desc") == 0 || strcmp(filename, "depends") == 0 || (strcmp(filename, "deltas") == 0 && db->handle->deltaratio > 0.0) ) { int ret; -- 1.8.3.2
participants (4)
-
Allan McRae
-
Dan McGee
-
Dave Reisner
-
Dave Reisner