[pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB

older
[pacman-dev] [PATCH 0/2] separate...

Dave Reisner

7 Jul 2013 7 Jul '13

11:53 p.m.

As seen: https://bbs.archlinux.org/viewtopic.php?pid=1297766 Signed-off-by: Dave Reisner --- 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

Show replies by date

Allan McRae

8 Jul 8 Jul

1:22 a.m.

New subject: [pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB

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.

...

Signed-off-by: Dave Reisner --- 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);

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.

...

+ return 0; + } + if(strcmp(filename, "desc") == 0 || strcmp(filename, "depends") == 0 || (strcmp(filename, "deltas") == 0 && db->handle->deltaratio > 0.0) ) { int ret;

Dave Reisner

1:39 a.m.

New subject: [pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB

On Mon, Jul 08, 2013 at 11:22:15AM +1000, Allan McRae wrote:

...

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.

Fair enough... and who knows, maybe the bbs will go away some day ;)

...

...
Signed-off-by: Dave Reisner --- 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);

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.

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.

...

...
+ return 0; + } + if(strcmp(filename, "desc") == 0 || strcmp(filename, "depends") == 0 || (strcmp(filename, "deltas") == 0 && db->handle->deltaratio > 0.0) ) { int ret;

Dave Reisner

1:46 a.m.

New subject: [pacman-dev] [PATCHv2] be_sync: avoid crashing on files in the root of a DB

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 --- 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

Dan McGee

2:52 p.m.

New subject: [pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB

On Sun, Jul 7, 2013 at 8:39 PM, Dave Reisner wrote:

...

On Mon, Jul 08, 2013 at 11:22:15AM +1000, Allan McRae wrote:

...
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.

Fair enough... and who knows, maybe the bbs will go away some day ;)

...
...
Signed-off-by: Dave Reisner --- 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);

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.

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.

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

Dave Reisner

3:03 p.m.

New subject: [pacman-dev] [PATCH] be_sync: avoid crashing on files in the root of a DB

On Mon, Jul 08, 2013 at 09:52:31AM -0500, Dan McGee wrote:

...

On Sun, Jul 7, 2013 at 8:39 PM, Dave Reisner wrote:

...
On Mon, Jul 08, 2013 at 11:22:15AM +1000, Allan McRae wrote:

...
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.

Fair enough... and who knows, maybe the bbs will go away some day ;)

...
...
Signed-off-by: Dave Reisner --- 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);

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.

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.

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.

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

Dave Reisner

9 Jul 9 Jul

8:07 p.m.

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 --- 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

3952

Age (days ago)

3954

Last active (days ago)

List overview

Download

6 comments

4 participants

participants (4)

Allan McRae
Dan McGee
Dave Reisner
Dave Reisner