[OPEN-ILS-DEV] PATCH: utils.c (miscellaneous)

[Bill Erickson]

> This patch is mostly a couple of tweaks to the growing_buffer code,
> loosely
> related to my previous patch to utils.h.  There is also a small tweak to
> uescape().
>
> 1. in buffer_add() I replaced strcat() with strcpy() for appending the new
> string.  Since we already know where the end of the old string is, we
> don't
> need to ask strcat() to find it for us.
>
> 2. In buffer_reset(), the old code contains the following:
>
>     osrf_clearbuf( gb->buf, sizeof(gb->buf) );
>
> The evident intent is to clear the buffer.  However sizeof(gb->buf) is not
> the size of the buffer, it's the size of the pointer to the buffer.  We
> were clearing only the first four bytes or so.  I changed the line to:
>
>     osrf_clearbuf( gb->buf, gb->size );
>
> 3. Also in buffer_reset(), I added a line to populate the first byte of
> the buffer with a nul, to ensure that the length of the (empty) string
> matches the n_used member.
>
> 4. In uescape(), we were examining the contents of string[] without first
> verifying that string was not NULL.  The result would be undefined
> behavior if string were ever NULL.  I added a couple of lines to treat
> a NULL pointer as if it were a pointer to an empty string.


Also applied.


>
> --------------
>
> Together with yesterday's patch to utils.h, these changes are intended
> to guarantee that the length of the string stored in a growing_buffer
> always matches the value of n_used.  buffer_data() relies on such a
> match.  So did buffer_add() (and, indirectly, buffer_fadd()) until I
> changed it.  So does every bit of code that calls buffer_release().
>
> Previously the terminal nuls were supplied by filling the entire buffer
> with nuls before otherwise populating it.  This practice always bothered
> me because it looked like a waste of CPU cycles.
>
> The use of osrf_clearbuf() in buffer_reset() is probably no longer
> necessary or useful.  However, from an abundance of caution I have left it
> in there.  I cannot guarantee that there is no other bit of code that
> fails
> to append a terminal nul where it should.  It might be a useful experiment
> to eliminate the prenullification and see if anything breaks.
>
> Another possible experiment is to insert the assert() macro at the entry
> and exit of each of the growing_buffer functions:
>
>     assert( strlen( gb->buf ) == gb->n_used );
>
> Such an assertion would add overhead for development and debugging, but
> would reduce to a no-op when NDEBUG is #defined.


I bet there are still some dragons lurking, so your caution is appreciated.

Thanks again,

-b

-- 
Bill Erickson
| VP, Software Development & Integration
| Equinox Software, Inc. / The Evergreen Experts
| phone: 877-OPEN-ILS (673-6457)
| email: erickson at esilibrary.com
| web: http://esilibrary.com