News:

Use the "Forum Search"
It may help you to find anything in the forum ;).

Server spamming listing server with multiple announce requests per second

Started by Ashley, March 20, 2014, 09:22:15 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

Ashley

I've seen this before, and another server ("sim.nova6.org") seems to be suffering from it now. I'm seeing announce requests occurring way too quickly, every half second or so which is way too quick (should be every 15 minutes at a minimum). The announce server isn't written to cope with that kind of loading (it runs on a free hosting service which is fairly limited, and this spam has overwhelmed the database service I was using so I've just disabled that now since it wasn't essential).

I'm not sure what version of the game the "sim.nova6.org" is running, but this issue was seen before some months ago so I presume it hasn't been fixed. Last time I just blocked that server by IP...

Previous thread about this: http://forum.simutrans.com/index.php?topic=11424.msg122424#msg122424
Use Firefox? Interested in IPv6? Try SixOrNot the IPv6 status indicator for Firefox.
Why not try playing Simutrans online? See the Game Servers board for details.

Dwachs

Cannot find anything suspicious in the code.

What values of the variables aiv and st did the server send per POST?
Parsley, sage, rosemary, and maggikraut.

TurfIt

Server up for more than 49.7 days ... rollover uint32 ms timer .... spam server...

Dwachs

The problem is that the posix dr_time function returns an uint64, all other dr_time implementations return uint32. The internal counter to check for announcing uses uint32.

The solution would be to force all dr_time functions to uint32. The SDL and Windows timer is per design restricted to 32bit:
http://wiki.libsdl.org/SDL_GetTicks
http://msdn.microsoft.com/en-us/library/windows/desktop/dd757629%28v=vs.85%29.aspx
Parsley, sage, rosemary, and maggikraut.

TurfIt

?? - all dr_times return unsigned long. When compiled 32-bit or 64-bit on Windows, this is 32 bit. When compiled 64-bit Linux or OSX, it's 64 bit and will create grief. IMHO variables should only be defined using the specific bit length declarations - uint32, uint64, sint8, etc. with the 'normal' int, long, short, etc. forbidden to avoid these platform specific differences.

Forcing all dr_times to uint32 will still result in the server being spammed during the rollover.  (dr_time() > last_time + interval) is true when (last_time + interval) > uint32_max. There's also (time - interval > last_time) constructs that are falsely true while time < interval.
Further, the code is rife with using dr_time values in expressions with unsigned longs, longs, sint32s, uint32s, and sint64s. The only consistency is the lack of rollover detection and handling.

Windows, Linux, and OSX each have a 64-bit timing routine that could be used instead. Not sure of the performance implications of calling those vs the 32-bit ones.
Or dr_time could be modified to extend the 32-bit OS calls to a sint64 return value.
Or dr_time could be changed to uint32 return, and all expressions using these values be modified to handle overflow.
Or the game could simply force quit after 49 days uptime...

Ters

Isn't the one true way to do this check current_time - last_time > interval? Then it should not matter what the data type is, as long as it's way bigger than interval.

TurfIt

And what happens when current_time is less than last_time? I.E. during unsigned rollover.

Ters

Quote from: TurfIt on March 23, 2014, 08:29:52 PM
And what happens when current_time is less than last_time? I.E. during unsigned rollover.

Hard to explain, so I'll show by example using unsigned 4-bit (assuming two's complement, which is quite safe for us):

interval: 0100
current_time: 0001
last_time: 1111
(current_time - last_time): 0010
(current_time - last_time) > interval = 0010 > 0100: false

I think signed should work as well, but my head is not quite into checking it.

TurfIt

Indeed. current - last > interval is the safe correct way.
Brain fart + smug comment == bitten idiot. Time to go hide again...

Ters

I must say I admire the people who found out such hacks back in the mists of time.

Quote from: TurfIt on March 23, 2014, 09:14:33 PM
Brain fart + smug comment == bitten idiot. Time to go hide again...

It didn't appear that smug, whatever the intention was. (Something that is a quite rare phenomenon on the Internet.)

Dwachs

here is a patch: changing dr_time to return uint32, and modifying all timer comparisons.

Please check whether this is ok
Parsley, sage, rosemary, and maggikraut.

Ters

It's not something that was introduced now, but the name of the variable called last_step in simmain.cc is rather confusing to me. I'd normally expect dr_time() >= last_something always (except at wrap-around). Perhaps a name with something like end or stop would make more sense.

TurfIt

I don't believe the use of sint32 works for this 'trick', especially when the full uint32 is being used. Doesn't that create issues when the one of the values is just over the positive signed maximum, and the other is still below?  Doing all the subtractions unsigned should return a small positive value in all cases, then using that in further signed calculations should be ok.

Ters

sint32 can make sense if it's not certain which time is the newest. If you do some_time = dr_time() + period, then dr_time() - some_time will be negative until period has elapsed. (Again assuming that period and the time elapsing between checks are way smaller than the range of the data type, in this case the signed data type.)

Dwachs

Quote from: TurfIt on April 03, 2014, 11:28:04 PM
I don't believe the use of sint32 works for this 'trick', especially when the full uint32 is being used. Doesn't that create issues when the one of the values is just over the positive signed maximum, and the other is still below?  Doing all the subtractions unsigned should return a small positive value in all cases, then using that in further signed calculations should be ok.
Could it be that these two expressions are equivalent:

sint32 time_to_next_step = (sint32)next_step_time - (sint32)ms;
// versus
sint32 time_to_next_step = (sint32)( next_step_time - ms );

?
Parsley, sage, rosemary, and maggikraut.

Ters

Quote from: Dwachs on April 04, 2014, 05:53:03 AM
Could it be that these two expressions are equivalent:

sint32 time_to_next_step = (sint32)next_step_time - (sint32)ms;
// versus
sint32 time_to_next_step = (sint32)( next_step_time - ms );

?

They should be equivalent on any computer using two's complement. In the x86 instruction set, there is no difference between signed and unsigned addition and subtraction. (Multiplication and division is another matter.) I think that only comes into play when deciding how to interpret the status flags, but C will only do that for comparisons. If some architecture throws exceptions on overflows, there might be some difference, but then we would be in trouble either way.

TurfIt

From what I've read, C only guarantees modular arithmetic for unsigned values. It's undefined for signed. i.e. UINT_MAX+1 wraps around. INT_MAX+1 is undefined.

Ters

Quote from: TurfIt on April 04, 2014, 08:12:44 PM
From what I've read, C only guarantees modular arithmetic for unsigned values. It's undefined for signed. i.e. UINT_MAX+1 wraps around. INT_MAX+1 is undefined.

Sounds likely, but that's also likely because C is more portable than the assumptions I made. As far as I can google, C does not require two's complement. Systems that don't use two's complement are however so rare that it's not likely to be an issue for Simutrans now or in the foreseeable future.

prissi

offtopic:
As far as I could google, the last computers with ones complement were C(yber)D(ata)C(ooperation) and some Univac. The latest built in 1985. Since one needs at least C99 to compile simutrans, it is safe to assume it is exticts (same as 9 bits per bytes and other stuff, which I used on a PDP9 ... )

Very offtopic:
But the IP header still use one complement.

Dwachs

Here is an updated patch, it seems to work with overflowing dr_time ...

Any comments?
Parsley, sage, rosemary, and maggikraut.

TurfIt

I still think all the arithmetic should be done using unsigned to be strictly correct, even if things seem to work with signed on current platforms/compilers.
Also, some time calcs have been missed. e.g. in stadt_t::step(). I just ran into this one while profiling Experimental, so it's fresh in my mind. I'm sure there's more lurking.

Dwachs

Ok, have to check simcity and all the functions taking delta_t arguments.

I used signed arithmetics only at places, where a decision has to be made whether 'time1 < time2'. This would completely fail for uint32-arithmetic due to the overflow problem, for signed arithmetic it works if abs(time1-time2) < 215.
Parsley, sage, rosemary, and maggikraut.

Dwachs

New and final patch. I checked simcity.cc and simfab.cc, both do not use absolute times but work internally with time difference, so danger of overflow.

The only point, where overflow could happen was convoy_t::go_on_ticks. I changed that, too.

Please look into the patch, and check the patch and/or the code to find more spots, where overflow could occur.
Parsley, sage, rosemary, and maggikraut.

TurfIt

Couple (long) casts to remove.
simworld.cc:4361:40: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
simworld.cc:4366:41: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

Dwachs

New version. This signed/unsigned comparison is removed
Parsley, sage, rosemary, and maggikraut.

Dwachs

Parsley, sage, rosemary, and maggikraut.

jamespetts

Hmm, perhaps a little late, but could one not use ticks for the announce server timings instead, and make ticks 64-bit (if it is not already)? Ticks has been 64-bit in Experimental for years and has worked fine.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.