11 Mar
2023
11 Mar
'23
5:13 a.m.
Howdy! I found a strange bug in the new version of Udpcast. When
sending packets at speeds over around 80m, the program locks up. I
traced the problem to the doRateLimit function in rate-limit.c (I've
added some printfs):
sleepTime = rateLimit->queueSize * 8 * MILLION /
rateLimit->bitrate;
printf("sleepTime1 %lu\n",sleepTime);
printf("rateLimit->queueSize %lu\n",rateLimit->queueSize);
if(sleepTime > 40000 || rateLimit->queueSize >= 100000) {
printf("sleepTime2 %lu\n",sleepTime);
sleepTime -= 10000;
printf("sleepTime3 %lu\n",sleepTime);
printf("sleepTime3 % 10000 %u\n",sleepTime % 10000);
sleepTime -= sleepTime % 10000;
printf("sleepTime4 %lu\n",sleepTime);
usleep((useconds_t)sleepTime);
}
What is happening is that sometimes rateLimit->queueSize >= 100000 but
sleepTime is small, so subtracting 10000 from it wraps around to near
maxint. The strange thing is that when the program then does sleepTime
-= sleepTime % 10000 the result is *different* from the 20200328 version
because of the change to long unsigned integers. In the old version
sleepTime would become zero, but now it stays huge, leading to an
extremely long usleep.
Here is the old behavior:
sleepTime1 1338
rateLimit->queueSize 100350
sleepTime2 1338
sleepTime3 4294958634
sleepTime3 % 10000 4294958634
sleepTime4 0
Here is the new behavior (which hangs):
sleepTime1 1344
rateLimit->queueSize 100800
sleepTime2 1344
sleepTime3 18446744073709542960
sleepTime3 % 10000 2960
sleepTime4 18446744073709540000
It looks to me like the older version actually had offsetting bugs:
sleepTime % 10000 *should* return a value less than 10000 but in the old
code for some reason it just returned sleepTime, which then subtracted
from itself gave zero. In the new version the wraparound error from the
sleepTime -= 10000 has been exposed, causing the hang.
Anyway, hopefully this isn't too hard to fix: your program is
super-useful for us, and the old version merrily blasts udp packets at
gigabit speeds!
Thanks,
Robert II
19 Mar
19 Mar
11:24 p.m.
Hi,
On 11/03/2023 06:13, Robert Theron Brockman II wrote:
Howdy! I found a strange bug in the new version of
Udpcast. When
sending packets at speeds over around 80m, the program locks up. I
traced the problem to the doRateLimit function in rate-limit.c (I've
added some printfs):
sleepTime = rateLimit->queueSize * 8 * MILLION / rateLimit->bitrate;
printf("sleepTime1 %lu\n",sleepTime);
printf("rateLimit->queueSize %lu\n",rateLimit->queueSize);
if(sleepTime > 40000 || rateLimit->queueSize >= 100000) {
printf("sleepTime2 %lu\n",sleepTime);
sleepTime -= 10000;
printf("sleepTime3 %lu\n",sleepTime);
printf("sleepTime3 % 10000 %u\n",sleepTime % 10000);
sleepTime -= sleepTime % 10000;
printf("sleepTime4 %lu\n",sleepTime);
usleep((useconds_t)sleepTime);
}
What is happening is that sometimes rateLimit->queueSize >= 100000 but
sleepTime is small, so subtracting 10000 from it wraps around to near
maxint. The strange thing is that when the program then does sleepTime
-= sleepTime % 10000 the result is *different* from the 20200328 version
because of the change to long unsigned integers. In the old version
sleepTime would become zero, but now it stays huge, leading to an
extremely long usleep.
Indeed. I've now fixed this by adding another comparison to make sure
that sleepTime is at least 20000, even if we are taking the branch due
to large queueSize. Smaller values than 20000 lead to 0 usleep (not a
catastrophe, but may throw timing off, because it still takes some time
just to do the system call), and smaller values than 10000 lead to
integer wraparound as you observed (worrysome, as this will lead to an
extremely long wait).
if(sleepTime > 40000 ||
(rateLimit->queueSize >= 100000 && sleepTime >= 20000 )) {
This is now in version 20230319
[...]
It looks to me like the older version actually had
offsetting bugs:
sleepTime % 10000 *should* return a value less than 10000 but in the old
code for some reason it just returned sleepTime, which then subtracted
from itself gave zero.
In the old version, sleepTime was declared as a signed integer (even
though a signed integer makes no sense as a parameter to usleep).
However, this had the side effect of modulo (%) giving a *negative*
result if sleeptime itself was negative. And this nicely brought it back
to 0, rather than causing the wraparound.
In the new version the wraparound error from the
sleepTime -= 10000 has been exposed, causing the hang.
Anyway, hopefully this isn't too hard to fix:
Indeed, the only thing needed was to add another test against 20000. I'm
even wondering whether replacing the entire condition with just
if(sleepTime > 20000) might work, but unfortunately this bit of code has
been there since the oldest version in subversion, so no commit comment
on why it was there.
your program is
super-useful for us, and the old version merrily blasts udp packets at
gigabit speeds!
I'm glad to hear that you like it, thanks,
Thanks,
Robert II
Regards,
Alain
9
days inactive
17
days old
1 comments
2 participants
participants (2)
-
Alain Knaff -
Robert Theron Brockman II