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

[Ivy Foster]

On 07 Sep 2016, at 10:05 pm -0400, Andrew Gregory wrote:
> On 09/07/16 at 07:22pm, ivy.foster at gmail.com wrote:
> > From: Ivy Foster <ivy.foster at gmail.com>

> This is a step in the right direction, but the problem of downloading
> an invalid db over a valid one still exists.  The errors given in the
> bug report are not actually from the invalid db, they're from invalid
> signature files.  If we download a junk db but no signature file, the
> valid db would still be overwritten by the invalid one.

Ah, my mistake. I should've actually looked at sync_db_validate; I
just assumed that it handled validation in general, not just
signatures. Also, unfortunately, I forgot about .files dbs.

> >  	/* Sanity checks */
> >  	ASSERT(db != NULL, return -1);
> > @@ -218,10 +221,23 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db)
> >  
> >  	dbext = db->handle->dbext;
> >  
> > +	len = strlen(syncpath) + strlen(db->treename) + strlen(dbext) + 2;
> > +	/* TODO fix leak syncpath and umask unset */
> > +	MALLOC(newdb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > +	snprintf(newdb, len, "%s/%s%s", syncpath, db->treename, dbext);
> > +	len += 4;
> > +	/* TODO fix leak syncpath and umask unset */
> > +	MALLOC(olddb, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> > +	snprintf(olddb, len, "%s.bak", newdb);
> 
> 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.

Thanks,
Ivy