<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40517 >
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.
> There seems to be another mistake though, and that is concerning
> the two uses of sizeof on a dynamically allocated array:
>
> 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).
[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?
Also, in loading map specials there is potential crash. I use
next change since first attemtps of loading games converted
from civ2 (aligned nmod was smaller than aligned S_LAST that time.)
@@ -4049,7 +4049,7 @@
special_order[j] = find_special_by_rule_name(modname[j]);
}
free(modname);
- for (; j < S_LAST + (4 - (S_LAST % 4)); j++) {
+ for (; j < S_LAST + (4 - (S_LAST % 4)) && j < nmod + (4 - (nmod % 4));
j++) {
special_order[j] = S_LAST;
}
}
But this is really for another ticket.
[/side note]
> > There should be some action after each of two added LOG_ERRORs.
> > One variant is to abort civserver. Opinions?
>
> If there's no easy way to handle the error after the error
> message (e.g. just dropping the orders for the unit)
Of course, it is. It's even already implemented several lines
above. Attached (with small change in symbol checking.)
--
Thanks, evyscr
Index: server/savegame.c
===================================================================
--- server/savegame.c (revision 15253)
+++ server/savegame.c (working copy)
@@ -1859,20 +1859,42 @@
/* Either ACTIVITY_FORTRESS or ACTIVITY_AIRBASE */
order->activity = ACTIVITY_BASE;
order->base = base_number(pbase);
- } else if (base_buf) {
- base = char2num(base_buf[j]);
+ } else if (order->activity == ACTIVITY_BASE) {
+ if (base_buf) {
+ if (strchr(num_chars, base_buf[j])) {
+ base = char2num(base_buf[j]);
- if (base >= 0
- && base < sizeof(base_order) / sizeof (struct base_type *)) {
- pbase = base_order[base];
- } else {
- freelog(LOG_ERROR, "Cannot find base %d for %s to build",
- base, unit_rule_name(punit));
- base = base_number(get_base_by_gui_type(BASE_GUI_FORTRESS, NULL, NULL));
- }
+ if (base >= 0
+ && base < sizeof(base_order) / sizeof (struct base_type *)) {
+ pbase = base_order[base];
+ } else {
+ freelog(LOG_ERROR, "Cannot find base %d for %s [%d] to build",
+ base, unit_rule_name(punit), punit->id);
+ base = base_number(get_base_by_gui_type(BASE_GUI_FORTRESS, NULL, NULL));
+ }
- order->base = base;
- }
+ order->base = base;
+ } else {
+ freelog(LOG_ERROR,
+ "%s [%d] has ACTIVITY_BASE but wrong base char"
+ " is in base_list. Dropping orders.",
+ unit_rule_name(punit), punit->id);
+ free(punit->orders.list);
+ punit->orders.list = NULL;
+ punit->has_orders = FALSE;
+ break;
+ }
+ } else {
+ freelog(LOG_ERROR,
+ "%s [%d] has ACTIVITY_BASE but no base_list present."
+ " Dropping orders.",
+ unit_rule_name(punit), punit->id);
+ free(punit->orders.list);
+ punit->orders.list = NULL;
+ punit->has_orders = FALSE;
+ break;
+ }
+ }
}
} else {
punit->has_orders = FALSE;
@@ -3470,7 +3492,7 @@
break;
}
}
- orders_buf[len] = dir_buf[len] = act_buf[len] = '\0';
+ orders_buf[len] = dir_buf[len] = act_buf[len] = base_buf[len] = '\0';
secfile_insert_str(file, orders_buf,
"player%d.u%d.orders_list", plrno, i);
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
