[Freeciv-Dev] Re: CVS's civserver core dumps

[Ben Webb <ben@xxxxxxxxxxxxxxxxxxxxxx>]

On Sat, 12 Jan 2002, Vasco Alexandre Da Silva Costa wrote:

> On Fri, 11 Jan 2002, Ben Webb wrote:
> >     Both this crash and the client crash reported by Paul seem to be 
> > caused by the generalised improvements code that Vasco committed yesterday 
> > and today. However, it is very different from my original code, so it's 
> > entirely possible that the changes have introduced bugs.

> The crash Paul found was in improvement_redundant(). IIRC i have not
> changed that function.

        Correct; however, the reason the function crashes is that it's 
passed an invalid player pointer. Since update_all_effects() gets that 
pointer from a bog-standard players_iterate, it's most likely that you've 
introduced a memory corruption bug in some other function. The original 
impr-gen patch didn't reveal such a bug in over nine months of use in 
FreecivAC.

> Since most of the patches consisted of fairly insulated code that merely
> constructed and updated some lists without it being used anywhere yet, i
> assumed they wouldn't impact things elsewhere. Unless there is a buffer
> overrun somewhere.

        My suspicions rest on get_effect_vectors() in 
common/improvement.c, which I have several severe problems with. (It seems 
to be my original get_effect_lists() function, with several bugs added.)

1. You no longer pass a player pointer; thus there is no way to exclude 
   Player- and Island- range effects any more. (I don't pass a player 
   pointer just to avoid a call to city_owner(), you know.) This breaks 
   impr-gen badly.
2. If there are multiple effects at a given range, you return the relevant 
   geff_vector multiple times, which is rather wasteful.
3. You return two variable length arrays, except that the first one is 
   always of length 1 (and you implicitly assume this when calling the 
   function). This seems rather bizarre.
4. The vectors are returned in the order in which the effects specify 
   them, rather than in order of increasing range. This breaks impr-gen 
   badly.
5. When you call the function, you pass in a geffs array of length 4 (this 
   is a seemingly hard-coded constant - what was wrong with EFR_LAST?) 
   Actually though, the returned geffs array is of dimension (number of 
   effects). This is a segfault just waiting to happen.

        Frankly, I don't quite understand why the original 
get_effect_lists() function wasn't used, which included none of these 
bugs. The patch has been available for testing since April, yet a greatly 
modified patch was committed to CVS without any public testing at all. Is 
it too much to ask that maintainers post their proposed changes to patches 
on freeciv-dev, if they're going to make more than simple "style" changes?

        Ben
-- 
ben@xxxxxxxxxxxxxxxxxxxxxx           http://bellatrix.pcl.ox.ac.uk/~ben/
"Computers in the future may weigh no more than 1.5 tons."
        - Popular Mechanics, forecasting the relentless march of science