[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