[streaming] FIXED: Flawed announcement interval computation in miniSAPserver causing SAP storms

[Zenon Mousmoulas]

On 20 Ιουν 2008, at 8:38 ΜΜ, A.L.M.Buxey at lboro.ac.uk wrote:

>
> :r324 | courmisch | 2007-03-11 14:10:50 +0200 (Sun, 11 Mar 2007) | 3  
> lines
> :Changed paths:
> :   M /miniSAPserver/trunk/sapserver.cpp
> :
> :Fix badly flawed announce delay computation.
> :Bug reported by Pierre Beyssac.
>
>
> the code itself:
>
>        unsigned n = config.Programs.size() ?: 1;
>        div_t d = div ((1000000000 / n) * config.GetDelay(),  
> 1000000000);
>        struct timespec delay;
>        delay.tv_sec = d.quot;
>        delay.tv_nsec = d.rem;
>
> i guess it could get b0rked by n...derived from Programs.size() being
> erroneous...


Hi,

Alan was almost right about this one...

The problem is actually that the numerator argument in the div()  
function is supposed to be an integer, but the expression used there  
quite easily overflows, for example if n=1 and config.GetDelay returns  
5, i.e. using a configuration that has sap_delay=5 and a single  
[program] section. The declaration of the operands is also a problem.  
Therefore d takes unpredictable values, which leads to an interval  
that is waaay off. My initial speculation that miniSAPserver couldn't  
be causing SAP storms on its' own was regrettably wrong. It can and  
will take your network down :)

This is certainly a critical issue that should be corrected, so I  
hacked two slightly different fixes (patches against 0.3.4 attached).  
The first one uses lldiv() and type casting. The second one just uses  
1000 instead of 1000000000, at the expense of lower precision. I would  
never need to set the interval at a better than millisecond precision  
though, so for me this is the preferred solution. In any case, I  
tested both and they take care of the problem.

Earlier today, and unfortunately after I had gone through the above, I  
literally stumbled across the repository for miniSAPserver:
svn://svn.videolan.org/network/miniSAPserver

I was trying to find what was actually changed in r324 to supposedly  
fix the delay computation, obviously without success.

Then I came across the last revision:

------------------------------------------------------------------------
r342 | courmisch | 2008-06-30 20:05:19 +0300 (Δευ, 30 Ιον 2008) |  
2 lines
Changed paths:
    M /miniSAPserver/trunk/NEWS
    M /miniSAPserver/trunk/sapserver.cpp

Fix timer computation for real

------------------------------------------------------------------------

Here is what was actually changed:

Index: trunk/sapserver.cpp
===================================================================
--- trunk/sapserver.cpp	(revision 341)
+++ trunk/sapserver.cpp	(revision 342)
@@ -240,7 +240,7 @@
          }

          unsigned n = config.Programs.size();
-        div_t d = div ((1000000000 / n) * config.GetDelay(),  
1000000000);
+        lldiv_t d = lldiv (1000000000LL * config.GetDelay() / n,  
1000000000);
          struct timespec delay;
          delay.tv_sec = d.quot;
          delay.tv_nsec = d.rem;

I tested it and I confirm this approach also works. I still prefer my  
hack #2 though :)

I can see that a new release 0.3.5 is in the works, which will include  
this fix. It hasn't been packaged and published yet though, so I had  
no clue this issue had already been addressed.

Anyway, this hopefully means fewer SAP storms for the future, or at  
least one less source for such unintentional storms...

Best regards,
Z.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: minisapserver-0.3.4-zmousm-fix1.diff
Type: application/octet-stream
Size: 1070 bytes
Desc: not available
Url : http://mailman.videolan.org/pipermail/streaming/attachments/20080709/3ae42c08/attachment.obj 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: minisapserver-0.3.4-zmousm-fix2.diff
Type: application/octet-stream
Size: 1149 bytes
Desc: not available
Url : http://mailman.videolan.org/pipermail/streaming/attachments/20080709/3ae42c08/attachment-0001.obj