Opened 2 years ago

Closed 2 years ago

#21971 closed defect (fixed)

Coverity issues in HS circuitmap unittests

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs prop224 coverity
Cc: Actual Points:
Parent ID: Points: 0.3
Reviewer: Sponsor: SponsorR-can

Description

We got two reports from coverity about the HS circuitmap unittests (#21889) which were merged in 0.3.1.

First one:

*** CID 1405129:    (NEGATIVE_RETURNS)
/src/test/test_hs_intropoint.c: 780 in test_received_introduce1_handling()
774       }
775     
776       /* Valid case. */
777       {
778         cell = helper_create_introduce1_cell();
779         ssize_t request_len = trn_cell_introduce1_encoded_len(cell);
>>>     CID 1405129:    (NEGATIVE_RETURNS)
>>>     Assigning: unsigned variable "print_" = "print1_".
780         tt_size_op(request_len, OP_GT, 0);

which is caused because we use tt_size_op to compare a ssize_t.


*** CID 1405130:    (REVERSE_INULL)
/src/test/test_circuitlist.c: 440 in test_hs_circuitmap_isolation()
434          * that token. */
435         tt_ptr_op(circ4, OP_EQ,
436                   hs_circuitmap_get_intro_circ_v2_service_side(tok2));
437       }
438     
439      done:
>>>     CID 1405130:    (REVERSE_INULL)
>>>     Null-checking "circ1" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
440       if (circ1)
441         circuit_free(TO_CIRCUIT(circ1));

which I think is caused because we NULL check circ1, even tho previous code is dereferencing it without any checks. Not sure what the right fix is here; perhaps we dont really need to NULL check it, and we can add a tt_assert(circ1) on the top as well.

Child Tickets

Change History (4)

comment:1 Changed 2 years ago by asn

Status: newneeds_review

Please see bug21971 for fixes to both of those issues.

Unfortunately, I didn't find a way to scan my branch and see if those fixes will actually 100% address those warnings, but I think it will work out.

comment:2 Changed 2 years ago by dgoulet

Hmmm, patch lgtm but I see possibly other type check issue like in test_establish_intro_wrong_purpose() where we have:

  ssize_t cell_len = 0;
  [...]
  tt_int_op(cell_len, >, 0);

Seems that Coverity doesn't bark on that one and I doubt it's harmful also...

comment:3 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

comment:4 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

You can also safely compare ssize_t with tt_i64_op.

Note: See TracTickets for help on using tickets.