[crossfire] Crossfire server code cleanup/janitorial

Tolga Dalman tolga.dalman at googlemail.com
Thu Apr 24 05:23:23 CDT 2014


> The standard "strncat" and "strncpy" functions do not guarantee that the 
> destination buffer will be null-terminated. This means that your changes 
> are vulnerable to many buffer overflow attacks.

Yes, but then those functions are used incorrectly. If the buffer is too
short, the cpy/cat operation will misbehave (i.e., the string will be
truncated).

> Using the sstring construct in Crossfire or moving to C++ will mitigate 
> this possibility, but probably not completely remove it.

I agree.

> Some of the code in the network loop should be cleaned up. Once I was also
> considering using something similar to SDL_Net, but decided that it wasn't
> worth the effort.

SDL Net is a fine portable socket abstraction and might solve some
immediate ugliness of the current networking code. Another possibility is
to go a step further and use frameworks like MessagePack or Protobuf
for serializing/deserializing byte streams and ZeroMQ as communication
framework. However, two problems come with that as well: as Mark noted
these libraries must be supported by all platforms, and such a change
would mean a considerable amount of work with little visible effects
for the user.

> I noticed many changes to silence compiler warnings. I haven't taken a 
> closer look, but perhaps it may be a good idea to change the function 
> definitions themselves instead of prefixing "(void)"?

That's what I did where possible. Elsewhere (esp. in types/) I couldn't
easily change the API.

> I also noticed a lot of changes that simply rip out compatibility in 
> 'porting.c', which is intended to keep that kind of stuff. Ideally, there
> would be no conditional compilations except there.

Assuming C99, these functions are not really needed, right ?

> Extend the above mentioned issue with ripping out the safe_* functions.

I haven't yet touched those, though I guess there could be done some
simplification as well (i.e., use tmpfile or mkstemp instead of tempnam*).

> I'll be working on cherry-picking specific changes, I hope your rebases 
> with master aren't too painful :)

Thank you very much! I really appreciate it.

> Consider using `git svn` which lets you keep track of the SVN repository.
> You can also rebase your work off my pre-cloned repository and track SVN
> from there.

Great hint, I didn't know about that before. I'll investigate.

Best regards
Tolga Dalman


More information about the crossfire mailing list