[pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

newer
[pacman-dev] "FATAL: kernel too...

Xavier

22 Jul 2007 22 Jul '07

2:06 p.m.

If Root isn't specified, paths like (null)var/ will be created. I think pacman should either default to / for Root, or just fail if it isn't set. Also, if no Cache Dirs is set, pacman will try to fallback to /tmp/ , but then it segfaults. (try pacman -Sw bar). Apparently it segfaults on this in libalpm/sync.c , sync_commit function, after downloading the file succesfully : 718 for(i = handle->dbs_sync; i; i = i->next) I found it weird that inside that loop, i was used a second time in an inner loop : 758 for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) { I thought that was the problem, but it didn't fix the segfault. Second strange thing is that the files list seem to be freed in any cases : 795 FREELIST(files); but it's used at the end of that _alpm_sync_commit function : 1018 if(!validcache && !(trans->flags & PM_TRANS_FLAG_DOWNLOADONLY)) { 1019 /* delete packages */ 1020 for(i = files; i; i = i->next) { 1021 unlink(i->data); 1022 } 1023 } Finally, if the cache isn't valid (so it fallbacks to /tmp/) and DOWNLOADONLY is used, files won't be deleted, but will they be used next times? Because the fallback (lines 779-790) happen after the check of existence of the package in the cache (lines 740-744). I can't check what happens because of the segfault.

Show replies by date

Dan McGee

22 Jul 22 Jul

4:27 p.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On 7/22/07, Xavier wrote:

...

If Root isn't specified, paths like (null)var/ will be created. I think pacman should either default to / for Root, or just fail if it isn't set. Also, if no Cache Dirs is set, pacman will try to fallback to /tmp/ , but then it segfaults. (try pacman -Sw bar).

Apparently it segfaults on this in libalpm/sync.c , sync_commit function, after downloading the file succesfully : 718 for(i = handle->dbs_sync; i; i = i->next)

I found it weird that inside that loop, i was used a second time in an inner loop : 758 for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) {

I thought that was the problem, but it didn't fix the segfault.

Second strange thing is that the files list seem to be freed in any cases : 795 FREELIST(files);

but it's used at the end of that _alpm_sync_commit function : 1018 if(!validcache && !(trans->flags & PM_TRANS_FLAG_DOWNLOADONLY)) { 1019 /* delete packages */ 1020 for(i = files; i; i = i->next) { 1021 unlink(i->data); 1022 } 1023 }

Finally, if the cache isn't valid (so it fallbacks to /tmp/) and DOWNLOADONLY is used, files won't be deleted, but will they be used next times? Because the fallback (lines 779-790) happen after the check of existence of the package in the cache (lines 740-744). I can't check what happens because of the segfault.

This is something I've been putting off but definitely needs doing. We need to do some checks both in the front end and back end for valid path specifications. I think hardcoding a default root of "/" is fine (or using the one specified at ./configure time)? Any thoughts here appreciated. I think it would be better to use those specified by configure, so I'm going to readd some of that stuff to the pacman front end this week otherwise the upgrade path will be a bit rough for most people. -Dan

Xavier

23 Jul 23 Jul

1:09 a.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On Sun, Jul 22, 2007 at 07:27:39PM -0400, Dan McGee wrote:

...

On 7/22/07, Xavier wrote: This is something I've been putting off but definitely needs doing. We need to do some checks both in the front end and back end for valid path specifications.

Maybe for all paths, there could be a sanity check in the backend, and a safe default in frontend ? That would fix the problem for RootDir at least. But for CacheDir isn't there a bigger problem, or is it just that I don't understand how that code works?

Andrew Fyfe

12:18 p.m.

Xavier wrote:

...

If Root isn't specified, paths like (null)var/ will be created. I think pacman should either default to / for Root, or just fail if it isn't set. The backend (although I think this is a frontend problem :) should do

if ( stat(rootdir, &buf) != 0 || ! S_ISDIR(&buf->st_mode) ) Ooops invalid root directory.... Doesn't the frontend already default to / or are you talking about a case where rootdir='' in pacman.conf? Andrew

Xavier

2:33 p.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On Mon, Jul 23, 2007 at 08:18:58PM +0100, Andrew Fyfe wrote:

...

Xavier wrote:

...
If Root isn't specified, paths like (null)var/ will be created. I think pacman should either default to / for Root, or just fail if it isn't set. The backend (although I think this is a frontend problem :) should do

if ( stat(rootdir, &buf) != 0 || ! S_ISDIR(&buf->st_mode) ) Ooops invalid root directory....

Doesn't the frontend already default to / or are you talking about a case where rootdir='' in pacman.conf?

It doesn't default to / anymore (in git), but I think it would be better if it did (maybe in the frontend). The case of an invalid rootdir in pacman.conf could be caught by a sanity check (maybe in the backend). Anyway, I'm not sure in which place they should be, but I believe that sane defaults + sanity checks is an important thing to have.

Dan McGee

2:38 p.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On 7/23/07, Xavier wrote:

...

On Mon, Jul 23, 2007 at 08:18:58PM +0100, Andrew Fyfe wrote:

...
Xavier wrote:

...
If Root isn't specified, paths like (null)var/ will be created. I think pacman should either default to / for Root, or just fail if it isn't set. The backend (although I think this is a frontend problem :) should do

if ( stat(rootdir, &buf) != 0 || ! S_ISDIR(&buf->st_mode) ) Ooops invalid root directory....

Doesn't the frontend already default to / or are you talking about a case where rootdir='' in pacman.conf?

It doesn't default to / anymore (in git), but I think it would be better if it did (maybe in the frontend). The case of an invalid rootdir in pacman.conf could be caught by a sanity check (maybe in the backend). Anyway, I'm not sure in which place they should be, but I believe that sane defaults + sanity checks is an important thing to have.

I'm starting to do some of this on my working branch- see my gitweb for details. -Dan

Xavier

20 Aug 20 Aug

10:32 a.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On Sun, Jul 22, 2007 at 11:06:44PM +0200, Xavier wrote:

...

Also, if no Cache Dirs is set, pacman will try to fallback to /tmp/ , but then it segfaults. (try pacman -Sw bar).

Apparently it segfaults on this in libalpm/sync.c , sync_commit function, after downloading the file succesfully : 718 for(i = handle->dbs_sync; i; i = i->next)

I found it weird that inside that loop, i was used a second time in an inner loop : 758 for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) {

I thought that was the problem, but it didn't fix the segfault.

Second strange thing is that the files list seem to be freed in any cases : 795 FREELIST(files);

but it's used at the end of that _alpm_sync_commit function : 1018 if(!validcache && !(trans->flags & PM_TRANS_FLAG_DOWNLOADONLY)) { 1019 /* delete packages */ 1020 for(i = files; i; i = i->next) { 1021 unlink(i->data); 1022 } 1023 }

Finally, if the cache isn't valid (so it fallbacks to /tmp/) and DOWNLOADONLY is used, files won't be deleted, but will they be used next times? Because the fallback (lines 779-790) happen after the check of existence of the package in the cache (lines 740-744). I can't check what happens because of the segfault.

The segfault still happened because this loop is there twice, but I only saw one occurence of it : for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) { And both times, it overlaps with the main loop. I guess Dan was tired when he wrote this ;) Anyway, I've been looking back at this code because of bug 6404 : http://bugs.archlinux.org/task/6404#comment17750 I've two questions : 1) is it very important to delete the packages when they are put in /tmp ? (the current code doing that is totally bogus as pointed out above, but I wonder if I couldn't just remove it instead of fixing it) 2) If I want to factor the "looking for package in cache" and "get a valide cache dir" codes, where would such functions belong ? I put them in libalpm/server.c , is this ok ?

Dan McGee

10:35 a.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On 8/20/07, Xavier wrote:

...

On Sun, Jul 22, 2007 at 11:06:44PM +0200, Xavier wrote:

...
Also, if no Cache Dirs is set, pacman will try to fallback to /tmp/ , but then it segfaults. (try pacman -Sw bar).

Apparently it segfaults on this in libalpm/sync.c , sync_commit function, after downloading the file succesfully : 718 for(i = handle->dbs_sync; i; i = i->next)

I found it weird that inside that loop, i was used a second time in an inner loop : 758 for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) {

I thought that was the problem, but it didn't fix the segfault.

Second strange thing is that the files list seem to be freed in any cases : 795 FREELIST(files);

but it's used at the end of that _alpm_sync_commit function : 1018 if(!validcache && !(trans->flags & PM_TRANS_FLAG_DOWNLOADONLY)) { 1019 /* delete packages */ 1020 for(i = files; i; i = i->next) { 1021 unlink(i->data); 1022 } 1023 }

Finally, if the cache isn't valid (so it fallbacks to /tmp/) and DOWNLOADONLY is used, files won't be deleted, but will they be used next times? Because the fallback (lines 779-790) happen after the check of existence of the package in the cache (lines 740-744). I can't check what happens because of the segfault.

The segfault still happened because this loop is there twice, but I only saw one occurence of it : for(i = alpm_option_get_cachedirs(); i; i = alpm_list_next(i)) {

And both times, it overlaps with the main loop. I guess Dan was tired when he wrote this ;)

I just fixed all this up in my tree today.

...

Anyway, I've been looking back at this code because of bug 6404 : http://bugs.archlinux.org/task/6404#comment17750

I've two questions :

1) is it very important to delete the packages when they are put in /tmp ? (the current code doing that is totally bogus as pointed out above, but I wonder if I couldn't just remove it instead of fixing it)

2) If I want to factor the "looking for package in cache" and "get a valide cache dir" codes, where would such functions belong ? I put them in libalpm/server.c , is this ok ?

Take a look at my tree. I made the decision on number 1 not to delete, and for number 2, I put them in util.c. -Dan

Xavier

10:59 a.m.

New subject: [pacman-dev] Missing sanity checks for path -> weird behaviors and segfaults

On Mon, Aug 20, 2007 at 01:35:52PM -0400, Dan McGee wrote:

...

Take a look at my tree. I made the decision on number 1 not to delete, and for number 2, I put them in util.c.

lol, amazing, that's exactly what I was doing, the code is very similar. So it's exactly like I wanted it to be, perfect ;) Just have to test it a bit now.

6088

Age (days ago)

6117

Last active (days ago)

List overview

Download

8 comments

3 participants

participants (3)

Andrew Fyfe
Dan McGee
Xavier