Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4283 closed defect (fixed)

crypto_pk_cmp_keys does not document its error behaviour

Reported by: rransom Owned by: rransom
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

/** Compare the public-key components of a and b.  Return -1 if a\<b, 0
 * if a==b, and 1 if a\>b.
 */
  if (!a || !b)
    return -1;

  if (!a->key || !b->key)
    return -1;

Can we replace these with asserts on 0.2.3.x?

Child Tickets

Change History (18)

comment:1 in reply to:  description Changed 8 years ago by nickm

Replying to rransom:

Can we replace these with asserts on 0.2.3.x?

Fine by me.

comment:2 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:3 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

Ick. I wanted to do this for 0.2.3.x, but at this point I don't have time to verify that every user of the function ensures that neither argument is NULL. Postponing. :(

comment:4 Changed 7 years ago by rransom

Someone should stick an LD_BUG log message in here, possibly on 0.2.3.x.

comment:5 Changed 7 years ago by rransom

From router_differences_are_cosmetic in src/or/routerlist.c:

      crypto_pk_cmp_keys(r1->onion_pkey, r2->onion_pkey) ||
      crypto_pk_cmp_keys(r1->identity_pkey, r2->identity_pkey) ||

Someday, this will need a comparison function which handles NULL keys properly (i.e. as equal to each other, and distinct from non-NULL keys). This call site can use an (in)equality comparison rather than an order comparison.

comment:6 Changed 7 years ago by nickm

Actually, I don't see why we shouldn't do that right now. (That is, define NULL==NULL, and NULL < everything else.)

Also, defining a crypto_pk_eq() function to wrap (0==crypto_pk_cmp_keys(a,b)) would probably help readability.

comment:7 in reply to:  6 Changed 7 years ago by arma

Replying to nickm:

Also, defining a crypto_pk_eq() function to wrap (0==crypto_pk_cmp_keys(a,b)) would probably help readability.

agree

comment:8 Changed 7 years ago by rransom

Owner: set to rransom
Status: newaccepted

comment:9 Changed 7 years ago by rransom

Status: acceptedneeds_review

See my bug4283 branch.

comment:10 Changed 7 years ago by rransom

Status: needs_reviewneeds_revision

On second thought, this isn't quite guaranteed to stay correct -- crypto_pk_cmp_keys is documented as returning -1, 0, or 1 (even though nothing relies on those exact values), and tor_memcmp isn't. (For the particular inputs it could operate on here, it does currently produce -1, 0, or 1, but that's a lucky accident.)

I'm in favor of changing the documented behaviour of crypto_pk_cmp_keys. If some later use would be easier if its result were clamped to 0 or ±1, that should be implemented as a separate function (preferably constant-time and in di_ops.c).

comment:11 Changed 7 years ago by nickm

Changing the behavior to return -1, 0, 1 instead of neg, zero, pos is fine by me; I agree with your reasoning.

comment:12 Changed 7 years ago by nickm

err, the other way around: changing the documentation to say neg, zero, pos is fine.

comment:13 Changed 7 years ago by rransom

Status: needs_revisionneeds_review

I've pushed a patch to change the documentation for crypto_pk_cmp_keys.

comment:14 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

I added a quick unit test in branch "bug4283" in my public repo, just to make sure of the new NULL behavior, and whoops! It looks like your implementation crashes on crypto_pk_cmp_keys(NULL, NULL).

comment:15 in reply to:  14 Changed 7 years ago by rransom

Status: needs_revisionneeds_review

Replying to nickm:

I added a quick unit test in branch "bug4283" in my public repo, just to make sure of the new NULL behavior, and whoops! It looks like your implementation crashes on crypto_pk_cmp_keys(NULL, NULL).

Oops. Fix pushed.

comment:16 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Squashed and merged and fixed a stupid error in my unit test (not in that order); thanks!

comment:17 Changed 7 years ago by nickm

Keywords: tor-client added

comment:18 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.