The International Simutrans Forum

 

Author Topic: Server spamming listing server with multiple announce requests per second  (Read 9618 times)

0 Members and 1 Guest are viewing this topic.

Offline Ashley

  • Coder/Patcher
  • Devotee
  • *
  • Posts: 1288
    • entropy.me.uk
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

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
Cannot find anything suspicious in the code.

What values of the variables aiv and st did the server send per POST?

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
Server up for more than 49.7 days ... rollover uint32 ms timer .... spam server...

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
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

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
?? - 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...

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
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.

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
And what happens when current_time is less than last_time? I.E. during unsigned rollover.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
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.

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
Indeed. current - last > interval is the safe correct way.
Brain fart + smug comment == bitten idiot. Time to go hide again...

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
I must say I admire the people who found out such hacks back in the mists of time.

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.)

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
here is a patch: changing dr_time to return uint32, and modifying all timer comparisons.

Please check whether this is ok

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
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.

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
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.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
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.)

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
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:
Code: [Select]
sint32 time_to_next_step = (sint32)next_step_time - (sint32)ms;
// versus
sint32 time_to_next_step = (sint32)( next_step_time - ms );
?

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
Could it be that these two expressions are equivalent:
Code: [Select]
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.

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
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.

Offline Ters

  • Coder/patcher
  • Devotee
  • *
  • Posts: 5531
  • Languages: EN, NO
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.

Offline prissi

  • Developer
  • Administrator
  • *
  • Posts: 9518
  • Languages: De,EN,JP
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.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
Here is an updated patch, it seems to work with overflowing dr_time ...

Any comments?

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
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.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
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.

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
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.

Offline TurfIt

  • Dev Team, Coder/patcher
  • Devotee
  • *
  • Posts: 1326
Couple (long) casts to remove.
Code: [Select]
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]

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
New version. This signed/unsigned comparison is removed

Offline Dwachs

  • DevTeam, Coder/patcher
  • Administrator
  • *
  • Posts: 4594
  • Languages: EN, DE, AT
incorporated into r7154

Offline jamespetts gb

  • Simutrans-Extended project coordinator
  • Devotee
  • *
  • Posts: 18721
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
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.