Re: [Linphone-developers] Bug and bug-pesticide in session_set_select() of oRTP-0.13.1 (multi-duplex-rtp-sessions problem)

[Simon Morlat]

Hi Serg,

I've read carefully your bug report and your bug-pesticide, and I admit you 
have found a real bug.
Indeed the temp SessionSet is not being reinitialised, thus during the 
session_set_copy, toggled fd from the recv result set may be put into the 
send result set...
By duplicating temp into temp1, temp2, temp3, you workaround the bug because 
probably your compiler initialise new stack variables with zeroee, which is 
not the case with all compilers, that's why I prefered solving the bug a bit 
differently by calling session_set_init(&temp) before each set computing. And 
of course it saves a bit of memory.
The fix will be on cvs tonight, and in next release of course.

Concerning the other points:
- rtp_session_resync(): probably not useful because rtp_session_recvm_with_ts 
or rtp_session_sendm_with_ts will do the job of setting/clearing the bit at 
next invocation. But maybe I'm wrong...

- sched->time_: does not need locking. The risk is only that you may have a 
RtpSession not in a recv or send set and need to wait 10 ms more to have it. 
Not a big issue. Anyway changing the mutex_unlock would not fix it, since the 
rtp_session_recvm_with_ts() does not take this mutex to check the value of 
sched->time_.

Congratulations and thanks for the bug investigation and fix !

Simon

Le Friday 16 November 2007 15:08:33 Машкин С В, vous avez écrit :
> Hi, Simon!
>
>
>
> It seems, that there is really bug in oRTP-0.13.1
>
>
>
> I have made experiments on x86-PC-platform (Linux OS)
>
> and on Blackfin-platform (uCLinux OS), and found this bug.
>
> Also I found the "pesticide" for the "bug", which
>
> helped.
>
>
>
> This bug is connected with multi-duplex-sessions
>
> and it crashes rtp-layer only when multi-duplex-sessions
>
> mode is used.
>
>
>
> The heart of the bug is (As it seems to me):
>
>
>
> when function session_set_select() is used, it must
>
>   1.return total number of rcv+send+error events
>
>   2.reset setted bits in sched->w_sessions,
>
>     sched->r_sessions sets
>
>   3.set/reset bits in recvs,sends,errors sets
>
>
>
> but current version (oRTP ver 0.13.1) of
>
> session_set_select does not correctly work with
>
>   3.set/reset bits in recvs,sends,errors sets
>
> (It is interesting, that you can see that only in
>
> multi-duplex-sessions mode - in other cases
>
> all works fine!)
>
>
>
> "PESTICIDE FOR BUG IN SESSION_SET_SELECT(), ORTP-0.13.1"
>
> ---------------------------------------------------------
>
> int session_set_select(SessionSet *recvs,
>
>                        SessionSet *sends,
>
>                        SessionSet *errors)
>
> {
>
>   int ret=0,bits;
>
>   SessionSet temp1,temp2,temp3;
>
>   RtpScheduler *sched=ortp_get_scheduler();
>
>
>
>   /*lock the scheduler to not read the masks while they
>
>   are being modified by the scheduler*/
>
>   rtp_scheduler_lock(sched);
>
>
>
>   while(1){
>
>
>
>   /* computes the SessionSet intersection (in the other
>
>   words mask intersection) between
>
>   the mask given by the user and scheduler masks */
>
>
>
>   if (recvs!=NULL){
>
>     bits=session_set_and(&sched->r_sessions,
>
>                          sched->all_max,recvs,&temp1);
>
>     if (bits>0){
>
>       ret+=bits;
>
>       /* copy the result set in the given user set */
>
>     }
>
>   }
>
>   if (sends!=NULL){
>
>     bits=session_set_and(&sched->w_sessions,
>
>                          sched->all_max,sends,&temp2);
>
>     if (bits>0){
>
>       ret+=bits;
>
>       /* copy the result set in the given user set */
>
>     }
>
>   }
>
>   if (errors!=NULL){
>
>     bits=session_set_and(&sched->e_sessions,
>
>                          sched->all_max,errors,&temp3);
>
>     if (bits>0){
>
>       ret+=bits;
>
>       /* copy the result set in the given user set */
>
>     }
>
>   }
>
>
>
>   if (ret>0){
>
>     if (recvs!=NULL)
>
>       session_set_copy(recvs,&temp1);
>
>     if (sends!=NULL)
>
>       session_set_copy(sends,&temp2);
>
>     if (errors!=NULL)
>
>       session_set_copy(errors,&temp3);
>
>
>
>     /* there are set file descriptors, return immediately */
>
>     //printf("There are %i sessions set, returning.\n",ret);
>
>     rtp_scheduler_unlock(sched);
>
>     return ret;
>
>   }
>
>   //printf("There are %i sessions set.\n",ret);
>
>   /* else we wait until the next loop of the scheduler*/
>
>   ortp_cond_wait(&sched->unblock_select_cond,&sched->lock);
>
>   }
>
>
>
>   return -1;
>
> }
>
> ---------------------------------------------------------
>
>
>
> Yes... I forgot about two notices:
>
>
>
> 1. Under Linux OS I used only WIN32 versions of session_sets
>
> functions and datatypes definitions ( ORTP_FD_ZERO,
>
> ORTP_FD_SET, ORTP_FD_CLR, ortp__fd_mask and so on),
>
> because I have no practice to work with Linux's fd_set's
>
> and I prefer always to know size of data-types (to be
>
> independent of hardware/os-platform). I don't know
>
> will all be good whith Linux fd_set's...
>
>
>
> 2. It is important (but not obviously) to call
>
> rtp_session_sendm_with_ts() and rtp_session_recvm_with_ts()
>
> functions at the "1st time" (i.e. after the 1st use
>
> of session_set_select() ) after session was made
>
> to be scheduled (Because session-times have to
>
> be initializated for correct working under
>
> scheduler and session_set_select controls).
>
>
>
> I am not shure, but It seems,
>
> that rtp_session_reset() rtp_session_resync() functions
>
> must do something with
>
> sched->r_sessions and sched->w_sessions sets,
>
> but not only set flags
>
> RTP_SESSION_SEND_NOT_STARTED and RTP_SESSION_RECV_NOT_STARTED
>
>
>
> And what about sched->time_ incrementing
>
> in rtp_scheduler_schedule()
>
> (Is it thread coflict or not? Because, it may be the
>
> additional bug).
>
>
>
> P.S.:
>
> Also I have sketch of working multi-duplex-session rtp
>
> application, which may be used as example, I think...
>
> But I don't know how to publish it on the forum
>
> without code-text-format problems...
>
> (If it is needed, I can send it in e-mail-attachment)
>
>
>
> Thanks,
>
> Serg Ma
>
>
> _______________________________________________
> Linphone-developers mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/linphone-developers