Opened 3 years ago

Closed 3 years ago

#19567 closed defect (fixed)

SR: Fix issues Coverity found:

Reported by: dgoulet Owned by:
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-sr test
Cc: asn Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor: SponsorR-must

Description

Issue 1:

/src/or/shared_random_state.c: 639 in disk_state_update()
633         next = &(line->next);
634       }
635       if (sr_state->current_srv != NULL) {
636         *next = line = tor_malloc_zero(sizeof(*line));
637         line->key = tor_strdup(dstate_cur_srv_key);
638         disk_state_put_srv_line(sr_state->current_srv, line);
>>>     CID 1362985:  Code maintainability issues  (UNUSED_VALUE)
>>>     Assigning value from "&line->next" to "next" here, but that stored value is overwritten before it can be used.
639         next = &(line->next);
640       }
641     
642       /* Parse the commits and construct config line(s). */
643       next = &sr_disk_state->Commit;
644       DIGESTMAP_FOREACH(sr_state->commits, key, sr_commit_t *, commit) {

Issue 2:

*** CID 1362984:  Memory - corruptions  (OVERRUN)
/src/test/test_shared_random.c: 943 in test_utils()
937         const char *payload =
938           "\x5d\xb9\x60\xb6\xcc\x51\x68\x52\x31\xd9\x88\x88\x71\x71\xe0\x30"
939           "\x59\x55\x7f\xcd\x61\xc0\x4b\x05\xb8\xcd\xc1\x48\xe9\xcd\x16\x1f"
940           "\x70\x15\x0c\xfc\xd3\x1a\x75\xd0\x93\x6c\xc4\xe0\x5c\xbe\xe2\x18"
941           "\xc7\xaf\x72\xb6\x7c\x9b\x52";
942         sr_commit_t commit1, commit2;
>>>     CID 1362984:  Memory - corruptions  (OVERRUN)
>>>     Overrunning buffer pointed to by "payload" of 56 bytes by passing it to a function which accesses it at byte offset 56 using argument "57UL". [Note: The source code implementation of the function has been overridden by a builtin model.]
943         memcpy(commit1.encoded_commit, payload, sizeof(commit1.encoded_commit));
944         memcpy(commit2.encoded_commit, payload, sizeof(commit2.encoded_commit));
945         tt_int_op(commitments_are_the_same(&commit1, &commit2), ==, 1);
946         /* Let's corrupt one of them. */
947         memset(commit1.encoded_commit, 'A', sizeof(commit1.encoded_commit));
948         tt_int_op(commitments_are_the_same(&commit1, &commit2), ==, 0);

Child Tickets

Change History (5)

comment:1 Changed 3 years ago by asn

Cc: asn added

comment:2 Changed 3 years ago by dgoulet

Status: newneeds_review

See branch bug19567_029_01. I made a third commit for a trivial comment change in shared_random.h.

comment:3 Changed 3 years ago by asn

Patch looks good.

One question. You seem to have removed setting *next to NULL there.

   next = &sr_disk_state->SharedRandValues;
-  *next = NULL;
   if (sr_state->previous_srv != NULL) {

Is there a reason for this removal? The coverity report does not seem related.

This pattern seems to also exist in other parts of the code (e.g. in entry_guards_update_state() and circuit_build_times_update_state(). Are we sure we don't need it?

It might be useful in case our state does not contain previous or current SRVs.

comment:4 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

Right the reason *next = NULL is there is to make sure that the "config" option is reset to empty if any error happens. In our case, we have the disk state reset prior that makes sure everything is NULLed out so we do not need that line.

comment:5 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.