Opened 6 years ago

Closed 6 years ago

#11519 closed defect (fixed)

uninitialized timeval causing valgrind errors

Reported by: robgjansen Owned by:
Priority: High Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.3-alpha
Severity: Keywords: tor-client 024-backport 023-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I noticed some valgrind errors while debugging Tor 0.2.5.2-alpha in Shadow. The problem still exists in Tor master as of today.

In circuituse.c, line 1518, the struct timeval old_timestamp_began; is declared, but never initialized before being used on line 1556:

control_event_circuit_cannibalized(circ, old_purpose,
                                         &old_timestamp_began);

Should old_timestamp_began have been set to circ->base_.timestamp_began before updating circ->base_.timestamp_began in line 1553?

Some valgrind backtraces:

==28186== Conditional jump or move depends on uninitialised value(s)
==28186==    at 0x3F2EC48DF9: vfprintf (vfprintf.c:1635)
==28186==    by 0x3F2EC74CB2: vasprintf (vasprintf.c:62)
==28186==    by 0x5FDEBEF: tor_vasprintf (compat.c:435)
==28186==    by 0x5EAC662: send_control_event_impl (control.c:615)
==28186==    by 0x5EA71C5: send_control_event (control.c:635)
==28186==    by 0x5EA75BF: control_event_circuit_status_minor (control.c:3586)
==28186==    by 0x5EA7649: control_event_circuit_cannibalized (control.c:3621)
==28186==    by 0x5F7A748: circuit_launch_by_extend_info (circuituse.c:1555)
==28186==    by 0x5F7CACA: circuit_get_open_circ_or_launch (circuituse.c:1844)
==28186==    by 0x5F7B725: connection_ap_handshake_attach_circuit (circuituse.c:2149)
==28186==    by 0x5F9C34E: connection_ap_make_link (connection_edge.c:2025)
==28186==    by 0x5EF2EF8: directory_initiate_command_rend (directory.c:1029)
==28186==  Uninitialised value was created by a stack allocation
==28186==    at 0x5E82580: ??? (in /tmp/I2JBEX-libshadow-plugin-scallion.so)
==28186== Conditional jump or move depends on uninitialised value(s)
==28186==    at 0x3F2ECB4DB1: __strftime_internal (strftime_l.c:993)
==28186==    by 0x3F2ECB6622: strftime_l (strftime_l.c:481)
==28186==    by 0x5FEAA47: format_iso_time (util.c:1500)
==28186==    by 0x5FEAA7C: format_iso_time_nospace (util.c:1508)
==28186==    by 0x5FEAAF9: format_iso_time_nospace_usec (util.c:1519)
==28186==    by 0x5EA7417: control_event_circuit_status_minor (control.c:3566)
==28186==    by 0x5EA7649: control_event_circuit_cannibalized (control.c:3621)
==28186==    by 0x5F7A748: circuit_launch_by_extend_info (circuituse.c:1555)
==28186==    by 0x5F7CACA: circuit_get_open_circ_or_launch (circuituse.c:1844)
==28186==    by 0x5F7B725: connection_ap_handshake_attach_circuit (circuituse.c:2149)
==28186==    by 0x5F9C34E: connection_ap_make_link (connection_edge.c:2025)
==28186==    by 0x5EF2EF8: directory_initiate_command_rend (directory.c:1029)
==28186==  Uninitialised value was created by a stack allocation
==28186==    at 0x5E82580: ??? (in /tmp/I2JBEX-libshadow-plugin-scallion.so)
==28186== Conditional jump or move depends on uninitialised value(s)
==28186==    at 0x5FE2296: correct_tm (compat.c:2559)
==28186==    by 0x5FE2516: tor_gmtime_r (compat.c:2673)
==28186==    by 0x5FEAA2A: format_iso_time (util.c:1500)
==28186==    by 0x5FEAA7C: format_iso_time_nospace (util.c:1508)
==28186==    by 0x5FEAAF9: format_iso_time_nospace_usec (util.c:1519)
==28186==    by 0x5EA7417: control_event_circuit_status_minor (control.c:3566)
==28186==    by 0x5EA7649: control_event_circuit_cannibalized (control.c:3621)
==28186==    by 0x5F7A748: circuit_launch_by_extend_info (circuituse.c:1555)
==28186==    by 0x5F7CACA: circuit_get_open_circ_or_launch (circuituse.c:1844)
==28186==    by 0x5F7B725: connection_ap_handshake_attach_circuit (circuituse.c:2149)
==28186==    by 0x5F9C34E: connection_ap_make_link (connection_edge.c:2025)
==28186==    by 0x5EF2EF8: directory_initiate_command_rend (directory.c:1029)
==28186==  Uninitialised value was created by a stack allocation
==28186==    at 0x5E82580: ??? (in /tmp/I2JBEX-libshadow-plugin-scallion.so)

Child Tickets

Change History (4)

comment:1 Changed 6 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.5.x-final
Priority: normalmajor

comment:2 Changed 6 years ago by nickm

Keywords: 024-backport 023-backport added
Status: newneeds_review

Any idea why we haven't seen this before? To me it looks like it was introduced by 88e0026d, which merged in 0.2.3.11-alpha.

Anyways, I have a fix against 0.2.3 in branch "bug11519_023"

comment:3 in reply to:  2 Changed 6 years ago by robgjansen

Replying to nickm:

Any idea why we haven't seen this before? To me it looks like it was introduced by 88e0026d, which merged in 0.2.3.11-alpha.

I have been noticing random failures in both 0.2.3.25 and 0.2.5.2-alpha, and more often in some of my larger scale Tor simulations (where small issues tend to be exacerbated because the thousands of instances of Tor increase code/state coverage). I ran valgrind in an attempt to locate the problem, and found this bug. My problems have appeared as random memory errors, and so I'm not sure if this bug is the answer I was looking for. More testing is needed to make that determination.

Anyways, I have a fix against 0.2.3 in branch "bug11519_023"

I tested precisely that fix and valgrind no longer reports the errors. Thanks!

comment:4 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay; merging this to 0.2.3 and forward. Thanks!

Note: See TracTickets for help on using tickets.