@pmatilai requested changes on this pull request.
> @@ -153,8 +158,14 @@ rpmPubkey rpmKeyringLookupKey(rpmKeyring keyring,
> rpmPubkey key)
if (keyring == NULL || key == NULL)
return NULL;
rdlock lock(keyring->mutex);
- auto item = keyring->keys.find(key->keyid);
- rpmPubkey rkey = item == keyring->keys.end() ? NULL :
rpmPubkeyLink(item->second);
+
+ auto range = keyring->keys.equal_range(key->keyid);
+ auto item = range.first;
+ for (; item != range.second; item++) {
When at all possible, keep the loop variable inside the for. It should also be
a reference, this is now making copies of the keys as it loops, and favor
pre-increment. And then the whole thing becomes something like
```
for (auto const & item = range.first; item != range.second; ++item) {
if (item->second->fp == key->fp)
return rpmPubkeyLink(item->second);
}
return NULL;
```
Alternatively, leave the rkey declaration before the loop, just initialized to
NULL and break out as needed, that's kinda matter of style.
> rdlock lock(keyring->mutex);
auto keyid = key2str(pgpDigParamsSignID(sig));
- auto item = keyring->keys.find(keyid);
- if (item != keyring->keys.end()) {
+
+ /* use first verifying key or last one with matching keyid */
+ auto range = keyring->keys.equal_range(keyid);
+ auto item = range.first;
+ for (; item != range.second; item++) {
Same as above, make item a reference inside the for-loop, pre-increment.
> /* Do the parameters match the signature? */
- if ((pgpDigParamsAlgo(sig, PGPVAL_PUBKEYALGO)
- != pgpDigParamsAlgo(pub, PGPVAL_PUBKEYALGO)) ||
- memcmp(pgpDigParamsSignID(sig), pgpDigParamsSignID(pub),
- PGP_KEYID_LEN)) {
+ if (pgpDigParamsAlgo(sig, PGPVAL_PUBKEYALGO)
+ != pgpDigParamsAlgo(key->pgpkey, PGPVAL_PUBKEYALGO)) {
key = NULL;
The condition needs to be on a different indentation level from the code block.
That part was wrong to begin with but this is a chance to fix it since we're
touching that code. Typical solutions:
- move the test to a helper variable or function to make it shorter
- push the second line of condition indentation deeper
- put the opening { on the next line as per "the other" coding style - this is
the one special case where is totally okay because it actually makes it quite
readable
>
-rpmRC rpmKeyringVerifySig2(rpmKeyring keyring, pgpDigParams sig, DIGEST_CTX
ctx, rpmPubkey * keyptr)
-{
- rpmRC rc = RPMRC_FAIL;
-
- if (sig && ctx) {
- pgpDigParams pgpkey = NULL;
- rpmPubkey key = findbySig(keyring, sig);
-
- if (key)
- pgpkey = key->pgpkey;
+ if (pgpVerifySignature(pgpkey, sig, ctx) == RPMRC_OK)
This doesn't seem right, we're now verifying the same signature twice. It's
expensive enough that we don't want to be doing that.
> }
- if (keyptr) {
- *keyptr = pubkeyPrimarykey(key);
+
+ /* keys but no one verifies: show errors from all */
+ if (range.first != range.second && !key) {
+ for (item = range.first; item != range.second; item++) {
+ key = item->second;
+ rc = pubkeyVerifySig(key, sig, ctx);
Don't reverify, just collect any errors on the first run and then log them all
if the result is a failure.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/3398#pullrequestreview-2384805905
You are receiving this because you are subscribed to this thread.
Message ID: <rpm-software-management/rpm/pull/3398/review/2384805...@github.com>_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
