<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40164 >
I looked through compiler warnings for release style build
(--disable-debug). Most were just warnings about unused variables
(used only when debugging enabled).
Two cases turned out to be real bugs:
----
Some dio_get_uintX() callers assume that dio_get_uintX() will always
set correct value to variable given via pointer. dio_get_uintX() may
fail and does not touch that variable at all. Then these callers
allocate memory based on that uninitialized variable!
Since dio_get_uintX() returns void, it's impossible to know if
variable is really set, or not touched at all.
There is three different models, that fix the problem:
1) Make dio_get_uintX() and a lot of functions using it to return
bool indicating success or not. Then make sure that all the callers
check the return value
2) Make sure all the callers initialize the variable with sensible
default before calling dio_get_uintX()
3) dio_get_uintX() initializes variable.
Attached patch implements solution 3. That way problem is not
reappearing if someone adds new call to dio_get_uintX() without
necessary checks.
This affects all branches
----
adjust_workers_button_pressed() is sending worker packets with
uninitialized coordinates, since code to set those coordinates is
inside assert() and thus not included to release build.
This affects S2_2 and TRUNK
- ML
- ML
diff -Nurd -X.diff_ignore freeciv/common/dataio.c freeciv/common/dataio.c
--- freeciv/common/dataio.c 2007-08-01 19:21:30.000000000 +0300
+++ freeciv/common/dataio.c 2008-03-23 21:24:36.000000000 +0200
@@ -402,7 +402,8 @@
}
/**************************************************************************
-...
+ Receive uint8 value to dest. In case of failure, value stored to dest
+ will be zero. Note that zero is legal value even when there is no failure.
**************************************************************************/
void dio_get_uint8(struct data_in *din, int *dest)
{
@@ -415,11 +416,14 @@
*dest = x;
}
din->current++;
+ } else if (dest) {
+ *dest = 0;
}
}
/**************************************************************************
-...
+ Receive uint16 value to dest. In case of failure, value stored to dest
+ will be zero. Note that zero is legal value even when there is no failure.
**************************************************************************/
void dio_get_uint16(struct data_in *din, int *dest)
{
@@ -432,11 +436,14 @@
*dest = ntohs(x);
}
din->current += 2;
+ } else if (dest) {
+ *dest = 0;
}
}
/**************************************************************************
-...
+ Receive uint32 value to dest. In case of failure, value stored to dest
+ will be zero. Note that zero is legal value even when there is no failure.
**************************************************************************/
void dio_get_uint32(struct data_in *din, int *dest)
{
@@ -449,6 +456,8 @@
*dest = ntohl(x);
}
din->current += 4;
+ } else if (dest) {
+ *dest = 0;
}
}
@@ -679,7 +688,7 @@
}
/**************************************************************************
-...
+ Receive vector of uint6 values.
**************************************************************************/
void dio_get_uint16_vec8(struct data_in *din, int **values, int stop_value)
{
diff -Nurd -X.diff_ignore freeciv/client/mapctrl_common.c freeciv/client/mapctrl_common.c
--- freeciv/client/mapctrl_common.c 2008-03-08 16:33:11.000000000 +0200
+++ freeciv/client/mapctrl_common.c 2008-03-24 00:50:32.000000000 +0200
@@ -568,8 +568,9 @@
if (pcity && !cma_is_city_under_agent(pcity, NULL)) {
int city_x, city_y;
+ bool success = city_base_to_city_map(&city_x, &city_y, pcity, ptile);
- assert(city_base_to_city_map(&city_x, &city_y, pcity, ptile));
+ assert(success);
if (NULL != tile_worked(ptile) && tile_worked(ptile) == pcity) {
dsend_packet_city_make_specialist(&client.conn, pcity->id,
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
