[OPEN-ILS-DEV] PATCH: socket_bundle.c (latent bugs)

[Scott McKellar]

1. In socket_open_unix_server() and socket_open_unix_client(), I added
checks to make sure that the path parameter doesn't point to aomething
too big to fit into the receiving buffer of the struct sockaddr_un.

2. In _socket_handle_client_data() I add a terminal nul to the data
received by recv().

-----------

At one time we were using memset() to pre-fill the buffer with nuls.
This was an inefficient but effective way to ensure the presence of a
terminal nul.

At some point the memset() was replaced by osrf_clearbuf(), a macro 
that either does the original memset() or fills the buffer with
exclamation points plus a terminal nul, depending on whether NDEBUG is
#defined.  In the latter case, the result will be that the input
message will be padded with exclamation points to a length of 1023.

Apparently this hasn't happened, or nobody noticed.

I suspect it never happened in testing, but may happen in production,
because I suspect that the conditional compilation is working
backwards from what was intended.  We define orsr_clearbuf() thus:

#ifndef NDEBUG
// The original ... replace with noop once no more errors occur in
NDEBUG mode
#define osrf_clearbuf( s, n ) memset( s, 0, n )
#else
#define osrf_clearbuf( s, n ) \
	do { \
		char * clearbuf_temp_s = (s); \
		size_t clearbuf_temp_n = (n); \
		memset( clearbuf_temp_s, '!', clearbuf_temp_n ); \
		clearbuf_temp_s[ clearbuf_temp_n - 1 ] = '\0'; \
	} while( 0 )
#endif

The normal convention is that NDEBUG means "debugging code is turned
off."  In particular, NDEBUG turns the standard assert() macro into
a no-op.

By that convention, the above macro uses the exclamation points
only when debugging is turned off, i.e. only in production code.

Surely that's not what was intended.  I believe we should be using
"#ifdef" rather than "#ifndef".  Of course it is also possible that
the makefiles use NDEBUG in the opposite of the usual sense.  Such 
a perverse practice would in principle be harmless as long as we
don't use the assert() macro -- and currently we don't.

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

Developer's Certificate of Origin 1.1 By making a contribution to
this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license indicated
in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source license
and I have the right under that license to submit that work with
modifications, whether created in whole or in part by me, under the
same open source license (unless I am permitted to submit under a
different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person
who certified (a), (b) or (c) and I have not modified it; and

(d) In the case of each of (a), (b), or (c), I understand and agree
that this project and the contribution are public and that a record
of the contribution (including all personal information I submit
with it, including my sign-off) is maintained indefinitely and may
be redistributed consistent with this project or the open source
license indicated in the file.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: socket_bundle_c_5.patch
Type: text/x-patch
Size: 1482 bytes
Desc: 434262263-socket_bundle_c_5.patch
Url : http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20080301/383ac4f1/socket_bundle_c_5.bin