On 28/09/2007, Dan Scott <[EMAIL PROTECTED]> wrote:
> Hi:
>
> snprintf is defined to print a maximum number of characters into a
> string, and guarantees to NIL-terminate the string.
>
> The following patch corrects several situations where a string was
> defined with 256 characters, but snprintf was called with only 255
> characters, meaning that the 256th character would never be reached.
> Given that the call to snprintf was preceded by bzero (a deprecated
> method of initializing all of the contents of a given string to NIL),
> it seems likely that snprintf's NIL-terminating behaviour was
> overlooked or forgotten about.
>
> As bzero is deprecated, this patch removes calls to bzero that are
> followed by snprintf calls to the same string; the maximum number of
> characters specified to snprintf has been corrected, as well.
>
> I think this is trivial enough not to require a DCO. Let me know if
> you feel otherwise.
>
Replying to myself because I slept on this a while longer...
The attached patch replaces the first patch in the thread. In the
attached patch, I:
a) replace bzero calls (bzero is deprecated) with calls to memset. I'm
initializing the memory to '\0', feel free to change that to 0 if you
prefer.
b) replace, as much as possible, static lengths in memset & snprintf
calls with sizeof calls instead; this way, if we decide to change the
length of a given string, we only have to update the value in one
place. So instead of:
char buf[256];
memset(buf, '\0', 256);
snprintf(buf, 256, ...
we use:
char buf[256];
memset(buf, '\0', sizeof(buf));
snprintf(bug, sizeof(buf), ...
c) removed calls to bzero() or memset() when the variable that was
being initialized is immediately being filled via snprintf() --
snprintf() guarantees that the string will be terminated with NIL.
I'm sending this to the list rather than just committing it, because
it's been a few years since I've exercised my C skills in any
significant way and I would appreciate a once-over from others (like
Scott!).
Would we be interested in migrating to safer functions like strlcpy
and strlcat in the future? This wouldn't affect the Evergreen code
much, but a quick grep over the OpenSRF code suggests that there is
enough usage that we could benefit from that. David Wheeler wrote
about strlcat / strlcpy in his Secure Programming for Linux HOWTO
(http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/library-c.html)
and I have some positive experiences from using them in the ibm_db2
PHP extension. I can pull together a patch for OpenSRF to migrate to
these functions in fairly quick order, if there is interest.
--
Dan Scott
Laurentian University
Index: Open-ILS/src/c-apps/oils_fetch.c
===================================================================
--- Open-ILS/src/c-apps/oils_fetch.c (revision 7851)
+++ Open-ILS/src/c-apps/oils_fetch.c (working copy)
@@ -50,8 +50,7 @@
osrfHashSet( fmClassMap, hint, apiname );
char method[256];
- bzero(method, 256);
- snprintf(method, 256, "open-ils.fetch.%s.retrieve", apiname);
+ snprintf(method, sizeof(method), "open-ils.fetch.%s.retrieve", apiname);
osrfAppRegisterMethod( MODULENAME,
method, "oilsFetchDoRetrieve", "", 1, 0 );
@@ -129,13 +128,11 @@
/* construct the SQL */
char sql[256];
- bzero(sql, 256);
- snprintf( sql, 255, "select * from %s.%s where id = %s;", schema, object, id );
+ snprintf( sql, sizeof(sql), "select * from %s.%s where id = %s;", schema, object, id );
/* find the object hint from the api name */
char hintbuf[256];
- bzero(hintbuf,256);
- snprintf(hintbuf, 255, "%s.%s", schema, object );
+ snprintf(hintbuf, sizeof(hintbuf), "%s.%s", schema, object );
char* hint = osrfHashGet( fmClassMap, hintbuf );
osrfLogDebug(OSRF_LOG_MARK, "%s SQL = %s", MODULENAME, sql);
Index: Open-ILS/src/c-apps/oils_cstore.c
===================================================================
--- Open-ILS/src/c-apps/oils_cstore.c (revision 7851)
+++ Open-ILS/src/c-apps/oils_cstore.c (working copy)
@@ -2970,7 +2970,7 @@
case DBI_TYPE_DATETIME :
- memset(dt_string, '\0', 256);
+ memset(dt_string, '\0', sizeof(dt_string));
memset(&gmdt, '\0', sizeof(gmdt));
memset(&_tmp_dt, '\0', sizeof(_tmp_dt));
@@ -3049,7 +3049,7 @@
case DBI_TYPE_DATETIME :
- memset(dt_string, '\0', 256);
+ memset(dt_string, '\0', sizeof(dt_string));
memset(&gmdt, '\0', sizeof(gmdt));
memset(&_tmp_dt, '\0', sizeof(_tmp_dt));
Index: Open-ILS/src/c-apps/oils_utils.c
===================================================================
--- Open-ILS/src/c-apps/oils_utils.c (revision 7851)
+++ Open-ILS/src/c-apps/oils_utils.c (working copy)
@@ -189,8 +189,7 @@
char* seed = jsonObjectGetString(o);
char* passhash = md5sum(passwd);
char buf[256];
- bzero(buf, 256);
- snprintf(buf, 255, "%s%s", seed, passhash);
+ snprintf(buf, sizeof(buf), "%s%s", seed, passhash);
char* fullhash = md5sum(buf);
jsonObjectFree(o);
Index: Open-ILS/src/c-apps/oils_event.c
===================================================================
--- Open-ILS/src/c-apps/oils_event.c (revision 7851)
+++ Open-ILS/src/c-apps/oils_event.c (working copy)
@@ -86,8 +86,8 @@
jsonObjectSetKey( json, "pid", jsonNewNumberObject(getpid()) );
char buf[256];
- memset(buf,0, 256);
- snprintf(buf, 256, "%s:%d", event->file, event->line);
+ memset(buf, '\0', sizeof(buf));
+ snprintf(buf, sizeof(buf), "%s:%d", event->file, event->line);
jsonObjectSetKey( json, "stacktrace", jsonNewObject(buf) );
if(event->perm) jsonObjectSetKey( json, "ilsperm", jsonNewObject(event->perm) );
Index: Open-ILS/src/apachemods/mod_xmlbuilder.c
===================================================================
--- Open-ILS/src/apachemods/mod_xmlbuilder.c (revision 7851)
+++ Open-ILS/src/apachemods/mod_xmlbuilder.c (working copy)
@@ -245,7 +245,7 @@
if(href[0] != '/') {
int len = strlen(ctx->xmlFile) + strlen(href) + 1;
char buf[len];
- bzero(buf, len);
+ memset( buf, '\0', sizeof(len) );
strcpy( buf, ctx->xmlFile );
int i;
for( i = strlen(buf); i != 0; i-- ) {
@@ -300,7 +300,7 @@
if( prop[0] == '&' && prop[nl-1] == ';' ) { /* replace the entity if we are one */
char buf[nl+1];
- bzero(buf, nl+1);
+ memset( buf, '\0', sizeof(buf) );
strncat(buf, prop + 1, nl - 2);
xmlEntityPtr ent = osrfHashGet( ctx->entHash, buf );
if(ent && ent->content) _prop = ent->content;
@@ -392,8 +392,8 @@
/* determine the path to the DTD file and load it */
int len = strlen(context->config->baseDir) + strlen(locale) + strlen(sysId) + 4;
- char buf[len]; bzero(buf,len);
- snprintf( buf, len, "%s/%s/%s", context->config->baseDir, locale, sysId );
+ char buf[len];
+ snprintf( buf, sizeof(buf), "%s/%s/%s", context->config->baseDir, locale, sysId );
xmlDtdPtr dtd = xmlParseDTD(NULL, buf);
if(!dtd) return;
Index: Open-ILS/src/apachemods/mod_xmlent.c
===================================================================
--- Open-ILS/src/apachemods/mod_xmlent.c (revision 7851)
+++ Open-ILS/src/apachemods/mod_xmlent.c (working copy)
@@ -202,7 +202,7 @@
static void XMLCALL charHandler( void* userData, const XML_Char* s, int len ) {
ap_filter_t* filter = (ap_filter_t*) userData;
char data[len+1];
- bzero(data, len+1);
+ memset( data, '\0', sizeof(data) );
memcpy( data, s, len );
xmlEntConfig* config = ap_get_module_config(
Index: Open-ILS/src/apachemods/mod_rest_gateway.c
===================================================================
--- Open-ILS/src/apachemods/mod_rest_gateway.c (revision 7851)
+++ Open-ILS/src/apachemods/mod_rest_gateway.c (working copy)
@@ -85,12 +85,12 @@
}
char body[1025];
- memset(body,0,1025);
+ memset(body, '\0', sizeof(body));
buffer = buffer_init(1025);
while(ap_get_client_block(r, body, 1024)) {
buffer_add( buffer, body );
- memset(body,0,1025);
+ memset(body, '\0', sizeof(body));
}
if(arg && arg[0]) {
