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

[Dan McGee]

On 8/20/07, Xavier <shiningxc at gmail.com> 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