[pacman-dev] [PATCH 1/2] alpm: check for invalid sync db before replacing original

[Ivy Foster]

On 08 Sep 2016, at 12:55 pm -0400, Andrew Gregory wrote:
> On 09/07/16 at 10:28pm, Ivy Foster wrote:
> > On 07 Sep 2016, at 10:05 pm -0400, Andrew Gregory wrote:

> > > This runs the risk of conflicting with another db if dbext is "" or
> > > ".bak", for example, if I have repos core and core.bak, syncing core
> > > would clobber the core.bak db.
> > 
> > Fair enough. I'll keep that in mind when addressing the note below.
> > 
> > > > +	if (rename(newdb, olddb) == -1) {
> > > > +		ret = -1;
> > > > +		handle->pm_errno = ALPM_ERR_DB_BACKUP;
> > > > +		goto cleanup;
> > > > +	}
> > > 
> > > This is backwards.  Instead of moving the good db out of the way and
> > > hoping we can move it back if something goes wrong, we should download
> > > the new database to a temp file then move it into place if it's good.
> > 
> > Okay. I'll change that around tomorrow. Given the note before this
> > one, perhaps the thing to do is to download the new db to a tmpfile
> > named by mkstemp.
> 
> As long as we're downloading directly into the db directory, ensuring
> we don't conflict with another db is impossible because, now that the
> extension is configurable, there are no limits on db file names.  Even
> a tmpfile could conflict if the conflicting db file hasn't been
> downloaded yet.  I see two ways around this:  place restrictions on db
> names/extensions or download into a separate directory and
> move/symlink them over once validated.
> 
> Using a separate directory with symlinks is probably a bit more fault
> tolerant because both the old and new versions of the db and its sig
> could coexist the entire time, but I doubt the small gain is worth the
> complexity.

> I am inclined to say that we should just reject database names
> beginning with '_' and use that as a prefix for any temporary files.
> That should be sufficient to prevent any conflicts and is trivial to
> check in register_syncdb.

I mean, the names returned by mkstemp are probably sufficient
protection. You provide a prefix and it provides six random characters
to follow. Personally, I'm willing to bet that nobody's using the
dbext ".db.tmp.XXXXXX", where each X is a random character...unless
they're deliberately *trying* to collide.

Either way, I'll have a look at it later this afternoon.

iff