<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40517 >
> [EMAIL PROTECTED] - Fri Oct 10 19:37:34 2008]:
>
> On Thu, 9 Oct 2008 Madeline Book wrote:
>
> > The missing '\0' at the end of base_buf seems to be the main
> > bug;
>
> No, all chars after strlen(order_list)-1 aren't processed. So this is
> just a non-orderliness, which doesn't affect game loading.
Since base_buf is allocated on the stack (auto) it contains
whatever garbage happened to be there before. Because of the
missing assignment of '\0', we end up calling strlen on a
non-terminated string (via secfile_insert_str, sbuf_strdup).
This will seek ahead in memory (on the stack) until it finds
a zero byte that it thinks is the terminator. Best case is that
this is inside base_buf already or just past the end of it, but
much more likely is that a bunch of random garbage gets saved
to the secfile. This is then stored in the savegame and may
cause loading to fail (e.g. if the garbage contained delimiters,
newlines, etc.).
> > sizeof(base_order) / sizeof(struct base_type *)
> >
> > Since base_order has type struct base_type ** the dividend
> > will always evaluate to the size of a pointer, which is the
> > same as the value of the divisor. So the whole expression will
> > always evaluate to one (which I think is not the desired
> > behaviour).
>
> Ooh... but I want to count this as another bug, let's go on with
> it in another ticket (and with author of this code).
>
> > I'm guessing that game.control.num_base_types should be used
> > instead, but even so this sounds suspicious since base_order
> > is allocated with size nmod + (4 - (nmod % 4)) where nmod is
> > loaded from the savegame (maybe nmod should be replaced by
> > game.control.num_base_types in the calloc call?).
>
> There may be a problem that bases list from savegame can differ
> from ruleset's one. I guess that game.control.num_base_types is
> loaded from ruleset (the only place where I can see it changed
> is in server/ruleset.c).
Maybe it would just solve a lot of these problems to have
base_order be a specvec<struct base_type *> (to borrow c++
template syntax). Then we would have the fast random access
of arrays, but also keep the maximum size information that
is needed in the place of the sizeof/sizeof expression.
> [side note]
> Speaking of nmod, am I right that if nmod % 4 == 0 aligning
> will add extra four positions? (nmod + 3 - (nmod - 1) % 4) should
> be nearest alignment, right?
Maybe it is nmod + (4 - (nmod % 4) to avoid calloc(0) when nmod
is zero. Though you are right that when nmod is divisible by 4
then it adds 4 positions.
It still bothers me that we have:
base_order = fc_calloc(nmod + (4 - (nmod % 4)), sizeof(*base_order));
for (j = 0; j < nmod; j++) {
base_order[j] = ...
for (; j < game.control.num_base_types + (4 -
(game.control.num_base_types % 4)); j++) {
base_order[j] = NULL;
The array base_order is being allocated in terms of nmod,
but is assumed to have a size in terms of num_base_types?
Sure they would appear to be the same most of the time, but
if nmod is less, it would seem to go past the end of the
memory for the array. :?
-----------------------------------------------------------------------
目玉をくり抜けば、朝飯前だぞ。
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
