Opened 13 years ago

Closed 13 years ago

#1827 closed Bug (fixed)

Race condition in tr_sharedInit()

Reported by: shiyao Owned by: charles
Priority: Normal Milestone: 1.51
Component: libtransmission Version: 1.50
Severity: Major Keywords:
Cc:

Description

In function tr_sharedInit (port-forwarding.c),

247: s->pulseTimer = tr_timerNew( session, sharedPulse, s, 1000 ); 248: s->isEnabled = isEnabled; 249: s->upnpStatus = TR_PORT_UNMAPPED; 250: s->natpmpStatus = TR_PORT_UNMAPPED;

Line 247 firstly invokes sharedPulse to set upnp and natpmp in function natPulse as following.

93 : oldStatus = tr_sharedTraversalStatus( s ); 94 : s->natpmpStatus = tr_natpmpPulse( s->natpmp, port, isEnabled ); 95 : s->upnpStatus = tr_upnpPulse( s->upnp, port, isEnabled ); 96 : newStatus = tr_sharedTraversalStatus( s ); 97 : 98 : if( newStatus != oldStatus ) 99 : tr_ninf( getKey( ), _( "State changed from \"%1$s\" to \"%2$s\"" ), 100: getNatStateStr( oldStatus ), 101: getNatStateStr( newStatus ) );

However, the order between line 248-250 and line 93-101 is random. This race may cause some consistent problems or undefined behavior. (1) upnp and natpmp are set and their status are TR_PORT_UNMAPPED; (2) upnp and natpmp are set and the printout information is wrong. ......

So I think line 248-257 should be put before line 247 that invokes sharedPulse.

Change History (7)

comment:1 Changed 13 years ago by shiyao

In function tr_sharedInit (port-forwarding.c),

247: s->pulseTimer = tr_timerNew( session, sharedPulse, s, 1000 );
248: s->isEnabled = isEnabled;
249: s->upnpStatus = TR_PORT_UNMAPPED;
250: s->natpmpStatus = TR_PORT_UNMAPPED;

Line 247 firstly invokes sharedPulse to set upnp and natpmp in function natPulse as following.

93 : oldStatus = tr_sharedTraversalStatus( s );
94 : s->natpmpStatus = tr_natpmpPulse( s->natpmp, port, isEnabled );
95 : s->upnpStatus = tr_upnpPulse( s->upnp, port, isEnabled );
96 : newStatus = tr_sharedTraversalStatus( s );
97 :
98 : if( newStatus != oldStatus )
99 : tr_ninf( getKey( ), _( "State changed from \"%1$s\" to \"%2$s\"" ),
100: getNatStateStr( oldStatus ),
101: getNatStateStr( newStatus ) );

However, the order between line 248-250 and line 93-101 is random. This race may cause some consistent problems or undefined behavior.
(1) upnp and natpmp are set and their status are TR_PORT_UNMAPPED;
(2) upnp and natpmp are set and the printout information is wrong.
......

So I think line 248-250 should be put before line 247 that invokes sharedPulse.

comment:2 Changed 13 years ago by charles

Why do you say the order between line 248-250 and line 93-101 is random?

comment:3 Changed 13 years ago by shiyao

I inserted delays in the codes to control the order and got the conclusion. These two sections race completely.

You can do a simple experiment. Insert,
(1) a short delay between 247 and 248;
(2) a long delay between 96 and 98;

Then line 247-249 will be executed in the gap of line 96-98.

comment:4 Changed 13 years ago by shiyao

You can also make other synchronizations to reorder these codes in any other order. The result is undetermined. I think it is a bug potentially. But I don't know its priority.

comment:5 Changed 13 years ago by charles

  • Milestone changed from None Set to 1.51
  • Priority changed from High to Normal
  • Status changed from new to assigned

Ah, I see why I was confused. In trunk, those two sections are only invoked in the same thread, so the race condition can't occur. But you're right, it is a possibility in 1.50.

comment:6 Changed 13 years ago by charles

  • Summary changed from Concurreny bug in function tr_sharedInit to Race condition in tr_sharedInit()

comment:7 Changed 13 years ago by charles

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in the 1.5x branch in r7897

Note: See TracTickets for help on using tickets.