[OPEN-ILS-DEV] C nits: string_array.c (Part 1)

[Scott McKellar]

I believe that there is at least one bug in the osrfStringArrayRemove 
function.

This function removes and frees a string from the pointer array, and
then shifts the remaining pointer to fill in the gap.  Here is the
code that does the shifting, where the subscript i initially refers 
to the string that we are removing:

	for( ; i != arr->size; i++ ) 
		arr->array[i] = arr->array[i+1];

	arr->size--;

Suppose that the array has 10 slots, and all of them are initially
occupied.  Hence arr->size == arr->arr_size == 10.  At the end of 
the above loop, we execute:

	arr->array[ 9 ] = arr->array[ 10 ];

Unfortunately arr->array[ 10 ] doesn't exist.  We are reading beyond
the end of the array.  Oops.

I believe that the following replacement code will fix the problem
(warning: uncompiled and untested):

	arr->size--;
	for( ; i < arr->size; i++ ) 
		arr->array[i] = arr->array[i+1];
	arr->array[i] = NULL;

This change will also fix a more subtle bug that may appear on
some systems and not others.

Even if the array isn't full, the last assignment in the loop
currently replaces a pointer with a pointer that was never valid.
In most systems, probably including the PINES system, that pointer
will be NULL.  However it is not guaranteed to be NULL.

We had originally allocated the array using the OSRF_MALLOC macro,
which fills the newly allocated memory with binary zeros.  In a
system where NULL is represented by all-bits-zero, the effect is
to initialize all the pointers in the array to NULL.

However there are systems where NULL is represented by some bit
pattern other than all-bits-zero.  The compiler is required to treat
the literal value 0 as equivalent to NULL whenever it appears in a
pointer context.  However the binary zeros applied by OSRF_MALLOC
do not appear in a pointer context.  They're just zeros.  As a 
consequence, in a system where NULL is not all-bits-zero, we
initialize the pointers to an invalid garbage value.

It gets worse.

I have read that in some architectures, pointers pass through a
special CPU register that validates the address.  If the pointer
is not a valid address, the program crashes, even if you're just
moving the pointer around without trying to dereference it.

There's no need to invoke undefined behavior when it can be so
easily avoided.

Scott McKellar
http://home.swbell.net/mck9/aargh/