[Rpm-maint] [PATCH] Add RPMTAG_IDENTITY calculation as tag extension

[Panu Matilainen]

On 04/03/2018 10:31 PM, Vladimir D. Seleznev wrote:
> RPMTAG_IDENTITY is calculating as digest of part of package header that
> does not contain irrelevant to package build tag entries.
> 
> Mathematically RPMTAG_IDENTITY value is a result of function of two
> variable: a package header and an rpm utility, thus this value can
> differ for same package and different version of rpm.
> 
> Despite tag entries are filtering by blacklist, rpm also filters tags
> that it doesn't know while it calculates identity value. So technically,
> it's a graylist.
> ---
>   lib/rpmtag.h  |  3 +-
>   lib/tagexts.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/rpmtag.h b/lib/rpmtag.h
> index 973a6b6..36dac85 100644
> --- a/lib/rpmtag.h
> +++ b/lib/rpmtag.h
> @@ -371,11 +371,12 @@ typedef enum rpmTag_e {
>       RPMTAG_PAYLOADDIGEST	= 5092, /* s[] */
>       RPMTAG_PAYLOADDIGESTALGO	= 5093, /* i */
>       RPMTAG_AUTOINSTALLED	= 5094, /* i reservation (unimplemented) */
> -    RPMTAG_IDENTITY		= 5095, /* s reservation (unimplemented) */
> +    RPMTAG_IDENTITY		= 5095, /* s extension */
>   
>       RPMTAG_FIRSTFREE_TAG	/*!< internal */
>   } rpmTag;
>   
> +#define	RPMTAG_IDENTITY_MAX_TAG		RPMTAG_FIRSTFREE_TAG
>   #define	RPMTAG_EXTERNAL_TAG		1000000
>   
>   /** \ingroup rpmtag
> diff --git a/lib/tagexts.c b/lib/tagexts.c
> index f72ff60..ca8ac35 100644
> --- a/lib/tagexts.c
> +++ b/lib/tagexts.c
> @@ -952,6 +952,98 @@ static int filenlinksTag(Header h, rpmtd td, headerGetFlags hgflags)
>       return (fc > 0);
>   }
>   
> +static int identityFilter(rpmTag tag)
> +{
> +    int ret = 0;
> +
> +    switch(tag) {
> +    case RPMTAG_ARCH:
> +    case RPMTAG_ARCHIVESIZE:
> +    case RPMTAG_AUTOINSTALLED:
> +    case RPMTAG_BUILDHOST:
> +    case RPMTAG_BUILDTIME:
> +    case RPMTAG_CHANGELOG:
> +    case RPMTAG_COOKIE:
> +    case RPMTAG_DBINSTANCE:
> +    case RPMTAG_DISTRIBUTION:
> +    case RPMTAG_DISTTAG:
> +    case RPMTAG_DISTURL:
> +    case RPMTAG_FILESTATES:
> +    case RPMTAG_HEADERIMMUTABLE:
> +    case RPMTAG_IDENTITY:
> +    case RPMTAG_INSTALLCOLOR:
> +    case RPMTAG_INSTALLTID:
> +    case RPMTAG_INSTALLTIME:

Many of these are only ever present in installed headers, and of such 
tags at least RPMTAG_ORIGDIRINDEXES, RPMTAG_ORIGBASENAMES and
RPMTAG_ORIGDIRNAMES should also be filtered out.

But see below...

> +    case RPMTAG_INSTFILENAMES:

RPMTAG_INSTFILENAMES is an extension and will never be part of a header, 
ditto for RPMTAG_IDENTITY in this implementation.

> +    case RPMTAG_LONGARCHIVESIZE:
> +    case RPMTAG_LONGSIGSIZE:
> +    case RPMTAG_LONGSIZE:
> +    case RPMTAG_PAYLOADDIGEST:

RPMTAG_PAYLOADDIGESTALGO too.

> +    case RPMTAG_RSAHEADER:
> +    case RPMTAG_SHA1HEADER:
> +    case RPMTAG_SIGLEMD5_1:
> +    case RPMTAG_SIGLEMD5_2:
> +    case RPMTAG_SIGMD5:
> +    case RPMTAG_SIGPGP:
> +    case RPMTAG_SIGSIZE:
> +    case RPMTAG_SIZE:

At least RPMTAG_SHA256HEADER, RPMSIGTAG_FILESIGNATURES and 
RPMSIGTAG_FILESIGNATURELENGTH should also be on this list.

Basically you'd want everything below HEADER_TAGBASE filtered out, with 
the possible exception of HEADER_I18NTABLE. And also you'd want to 
filter out everything that was added to the header as a part of 
installation. There's a better way to all those goals than list 
individual tags:

Grab the immutable region of the header, and do the iteration on that 
instead of the header you got as an argument to identityTag(). Something 
like (untested):

static int identityTag(Header h, rpmtd td, headerGetFlags hgflags)
{
     rpmtd_s itd;
     Header ih;

     /* rpm v3 packages dont have an immutable region */
     if (!headerGet(h, RPMTAG_HEADERIMMUTABLE, &itd, HEADERGET_DEFAULT))
         goto err;

     ih = headerImport(itd.data, itd.count, HEADERIMPORT_FAST);

     /* ... and then iterate + copy over that */
     HeaderIterator hi = headerInitIterator(h);
     while (headerNext(hi, &ttd) && rc) {
     ...
}

> +       ret = 1;
> +       break;
> +    default:
> +       break;
> +    }
> +
> +    if (tag >= RPMTAG_IDENTITY_MAX_TAG)
> +	ret = 1;
> +
> +    return ret;
> +}
> +
> +static int identityTag(Header h, rpmtd td, headerGetFlags hgflags)
> +{
> +    int rc = 1;
> +    unsigned int bsize;
> +    void * hb;
> +    DIGEST_CTX ctx;
> +    char * identity = NULL;
> +
> +    struct rpmtd_s ttd;
> +    Header ih = headerNew();
> +    HeaderIterator hi = headerInitIterator(h);
> +
> +    while (headerNext(hi, &ttd) && rc) {
> +	if (rpmtdCount(&ttd) > 0) {
> +	    if (!identityFilter(ttd.tag))
> +		rc = headerPut(ih, &ttd, HEADERPUT_DEFAULT);
> +	}
> +	rpmtdFreeData(&ttd);
> +    }
> +    headerFreeIterator(hi);
> +
> +    if (!rc)
> +	return 0;
> +
> +    hb = headerExport(ih, &bsize);
> +
> +    if (hb == NULL)
> +	return 0;

These early returns would leak the "ih" header. In general, we prefer 
functions to have one point of return where all function-level 
allocations are freed and early failure points jump to the exit point 
with goto. That way there's less chance for these kinds of leaks.

> +
> +    ctx = rpmDigestInit(PGPHASHALGO_SHA256, RPMDIGEST_NONE);

rpmDigestInit() can fail, you should check for errors.

> +    rpmDigestUpdate(ctx, hb, bsize);
> +    rpmDigestFinal(ctx, (void **) &identity, NULL, 1);
> +    headerFree(ih);
> +
> +    td->data = xstrdup(identity);
> +    td->type = RPM_STRING_TYPE;
> +    td->count = 1;
> +    td->flags = RPMTD_ALLOCED;
> +
> +    if (identity != NULL)
> +	identity = _free(identity);

Don't bother checking for NULL when freeing, free() will do that anyway.

Overall I do like this a whole lot better than the earlier version.

	- Panu -