[PATCH 21.5] Static checker fun

PATCH 21.5 I get to play with static checkers this summer. Here's some stuff that some code analysis tools I'm working with found in our code base. There were tons of false positives for various reasons, too. Locating these took some digging through the voluminous output. One thing I hope to accomplish is finding ways of making the checker shut up about good code that it isn't smart enough to understand. Here are some brief descriptions about what is going on and why I think this patch is correct. If there is anything controversial in here, I'll split it out as a separate patch. lwlib-Xlw.c: The initializer for 'val' dereferences 'instance'. Following that, we check whether 'val' is NULL and return without doing anything if so. That's backwards. xlwmenu.c: In the do-while loop starting on line 3662, if !parent, then root and waste do not get initialized (via a call to XQueryTree), and so comparing parent to root in the loop condition is bad, as is checking whether waste is non-NULL in order to XFree it. dgif_lib.c: The code sets 'Private' by dereferencing 'GifFile', then checks whether 'GifFile' is NULL. That's backwards. Also, if we fail to allocate memory for 'Object->Colors', then we return without deallocating 'Object'. fileio.c: Unfortunately, you cannot see line 2854 in the patch. On that line, fd is set to -1. Therefore, it can never be the case that fd >= 0. input-method-xlib.c: The function starts by dereferencing f, then xic, in variable initializers, then checks whether either of the two variables is NULL. Backwards again! md5.c: If Lstream_read encounters an error, it returns -1. This code is only checking for a zero return value. nas.c: In every place where the Err() macro is used, the local variable wi contains the only pointer to allocated storage. scrollbar-x.c: Both SCROLLBAR_X_NAME and SCROLLBAR_X_WIDGET dereference 'instance->scrollbar_data', but then we check whether that is NULL at the very end of the function. text.c: This is actually more work than necessary. We only need to move the very first assertion, in order to compare ei against NULL. I just moved the other two because they neither affect nor are affected by the intervening block of code, and you might as well fail early if you are going to fail. Actually, this still isn't strong enough. We should probably add: assert ((off < 0 || len < 0) == (ei != 0)); but I don't understand this code and would like the opinion of someone who does. vdb-posix.c: ABORT() can return (under unusual circumstances, admittedly, but it can), but this code assumes it cannot. Initialize signal_name to the empty string to guard against that. Actually, we probably ought to audit all uses of ABORT() and make sure they can handle a return. window.c: CURCHARSIZE expands to a call to either window_char_width or window_char_height, depending on the value of widthflag. Since the code visible in this patch is inside of a 'if (widthflag) { ... }' block, there is really no point in making that comparison. lwlib/ChangeLog addition: 2006-06-16 Jerry James <james(a)xemacs.org&gt; * lwlib-Xlw.c (xlw_scrollbar_callback): Do not dereference instance before checking whether it is NULL. * xlwmenu.c (xlw_map_menu): Prevent uninitialized access to root and waste. src/ChangeLog addition: 2006-06-16 Jerry James <james(a)xemacs.org&gt; * dgif_lib.c (DGifCloseFile): Do not dereference GifFile before checking if it is NULL. Also fix a memory leak. * fileio.c (Finsert_file_contents_internal): Remove dead code. * input-method-xlib.c (XIM_SetGeometry): Do not dereference f or xic before checking if they are NULL. * md5.c (Fmd5): Check whether Lstream_read encountered an error. * nas.c (Err): Fix a memory leak. * scrollbar-x.c (x_free_scrollbar_instance): Do not dereference instance->scrollbar_data before checking if it is NULL. * text.c (eicmp_1): Move assertions to before the point where they must be true for correctness. * vdb-posix.c (vdb_fault_handler): Guard against a return from ABORT(). * window.c (change_window_height): Skip always true comparison in the expansion of CURCHARSIZE. xemacs-21.5 source patch: Diff command: cvs -q diff -uN Files affected: src/window.c src/vdb-posix.c src/text.c src/scrollbar-x.c src/nas.c src/md5.c src/input-method-xlib.c src/fileio.c src/dgif_lib.c lwlib/xlwmenu.c lwlib/lwlib-Xlw.c Index: lwlib/lwlib-Xlw.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/lwlib/lwlib-Xlw.c,v retrieving revision 1.15 diff -d -u -r1.15 lwlib-Xlw.c --- lwlib/lwlib-Xlw.c 2005/11/26 11:45:59 1.15 +++ lwlib/lwlib-Xlw.c 2006/06/16 21:06:55 ＠＠ -158,13 +158,13 ＠＠ XlwScrollBarCallbackStruct *data = (XlwScrollBarCallbackStruct *) call_data; scroll_event event_data; - scrollbar_values *val = - (scrollbar_values *) instance->info->val->scrollbar_data; + scrollbar_values *val; double percent; if (!instance || widget->core.being_destroyed) return; + val = (scrollbar_values *) instance->info->val->scrollbar_data; id = instance->info->id; percent = (double) (data->value - 1) / (double) (INT_MAX - 1); Index: lwlib/xlwmenu.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/lwlib/xlwmenu.c,v retrieving revision 1.41 diff -d -u -r1.41 xlwmenu.c --- lwlib/xlwmenu.c 2006/05/12 19:25:29 1.41 +++ lwlib/xlwmenu.c 2006/06/16 21:06:55 ＠＠ -3630,8 +3630,8 ＠＠ if (!mw->menu.pointer_grabbed) { XWindowAttributes ret; - Window parent,root; - Window *waste; + Window parent,root = 0UL; + Window *waste = NULL; unsigned int num_waste; lw_menu_active = True; Index: src/dgif_lib.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/dgif_lib.c,v retrieving revision 1.13 diff -d -u -r1.13 dgif_lib.c --- src/dgif_lib.c 2004/05/06 12:12:13 1.13 +++ src/dgif_lib.c 2006/06/16 21:06:55 ＠＠ -366,10 +366,11 ＠＠ ******************************************************************************/ int DGifCloseFile(GifFileType *GifFile) { - GifFilePrivateType *Private = (GifFilePrivateType *)GifFile->Private; + GifFilePrivateType *Private; if (GifFile == NULL) return -1; + Private = (GifFilePrivateType *)GifFile->Private; if (!IS_READABLE(Private)) { /* This file was NOT open for reading: */ ＠＠ -929,8 +930,10 ＠＠ return((ColorMapObject *)NULL); Object->Colors = (GifColorType *)calloc(ColorCount, sizeof(GifColorType)); - if (Object->Colors == (GifColorType *)NULL) + if (Object->Colors == (GifColorType *)NULL) { + free(Object); return((ColorMapObject *)NULL); + } Object->ColorCount = ColorCount; Object->BitsPerPixel = BitSize(ColorCount); Index: src/fileio.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/fileio.c,v retrieving revision 1.104 diff -d -u -r1.104 fileio.c --- src/fileio.c 2005/01/28 02:36:24 1.104 +++ src/fileio.c 2006/06/16 21:06:55 ＠＠ -2855,7 +2855,6 ＠＠ if (qxe_stat (XSTRING_DATA (filename), &st) < 0) { - if (fd >= 0) retry_close (fd); badopen: if (NILP (visit)) report_file_error ("Opening input file", filename); Index: src/input-method-xlib.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/input-method-xlib.c,v retrieving revision 1.21 diff -d -u -r1.21 input-method-xlib.c --- src/input-method-xlib.c 2005/01/24 23:33:59 1.21 +++ src/input-method-xlib.c 2006/06/16 21:06:55 ＠＠ -384,13 +384,18 ＠＠ void XIM_SetGeometry (struct frame *f) { - XIC xic = FRAME_X_XIC (f); - XIMStyle style = FRAME_X_XIC_STYLE (f); + XIC xic; + XIMStyle style; XRectangle area; - if (!xic || !f) + if (!f) return; + xic = FRAME_X_XIC (f); + if (!xic) + return; + + style = FRAME_X_XIC_STYLE (f); if (style & XIMStatusArea) { /* Place Status Area in bottom right corner */ Index: src/md5.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/md5.c,v retrieving revision 1.15 diff -d -u -r1.15 md5.c --- src/md5.c 2002/06/05 09:56:26 1.15 +++ src/md5.c 2006/06/16 21:06:55 ＠＠ -556,7 +556,7 ＠＠ Ibyte tempbuf[1024]; /* some random amount */ Bytecount size_in_bytes = Lstream_read (XLSTREAM (instream), tempbuf, sizeof (tempbuf)); - if (!size_in_bytes) + if (size_in_bytes <= 0) break; /* Process the bytes. */ Index: src/nas.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/nas.c,v retrieving revision 1.18 diff -d -u -r1.18 nas.c --- src/nas.c 2004/11/04 23:06:43 1.18 +++ src/nas.c 2006/06/16 21:06:55 ＠＠ -728,7 +728,7 ＠＠ /* Stuff taken from wave.c from NAS. Just like snd files, NAS can't read wave data from memory, so these functions do that for us. */ -#define Err() { return NULL; } +#define Err() { free(wi); return NULL; } #define readFourcc(_f) dread(_f, sizeof(RIFF_FOURCC), 1) #define cmpID(_x, _y) \ strncmp((CBinbyte *) (_x), (CBinbyte *) (_y), sizeof(RIFF_FOURCC)) Index: src/scrollbar-x.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/scrollbar-x.c,v retrieving revision 1.31 diff -d -u -r1.31 scrollbar-x.c --- src/scrollbar-x.c 2005/10/25 08:32:49 1.31 +++ src/scrollbar-x.c 2006/06/16 21:06:55 ＠＠ -72,19 +72,21 ＠＠ static void x_free_scrollbar_instance (struct scrollbar_instance *instance) { - if (SCROLLBAR_X_NAME (instance)) - xfree (SCROLLBAR_X_NAME (instance), char *); - - if (SCROLLBAR_X_WIDGET (instance)) + if (instance->scrollbar_data) { - if (XtIsManaged (SCROLLBAR_X_WIDGET (instance))) - XtUnmanageChild (SCROLLBAR_X_WIDGET (instance)); + if (SCROLLBAR_X_NAME (instance)) + xfree (SCROLLBAR_X_NAME (instance), char *); - lw_destroy_all_widgets (SCROLLBAR_X_ID (instance)); - } + if (SCROLLBAR_X_WIDGET (instance)) + { + if (XtIsManaged (SCROLLBAR_X_WIDGET (instance))) + XtUnmanageChild (SCROLLBAR_X_WIDGET (instance)); - if (instance->scrollbar_data) - xfree (instance->scrollbar_data, void *); + lw_destroy_all_widgets (SCROLLBAR_X_ID (instance)); + } + + xfree (instance->scrollbar_data, void *); + } } /* A device method. */ Index: src/text.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/text.c,v retrieving revision 1.26 diff -d -u -r1.26 text.c --- src/text.c 2005/09/27 05:29:44 1.26 +++ src/text.c 2006/06/16 21:06:56 ＠＠ -2138,7 +2138,11 ＠＠ Bytecount len, Charcount charlen, const Ibyte *data, const Eistring *ei2, int is_ascii, int fold_case) { + assert ((data == 0) != (ei == 0)); + assert ((is_ascii != 0) == (data != 0)); + assert (fold_case >= 0 && fold_case <= 2); assert ((off < 0) != (charoff < 0)); + if (off < 0) { off = charcount_to_bytecount (ei->data_, charoff); ＠＠ -2152,9 +2156,6 ＠＠ assert (off >= 0 && off <= ei->bytelen_); assert (len >= 0 && off + len <= ei->bytelen_); - assert ((data == 0) != (ei == 0)); - assert ((is_ascii != 0) == (data != 0)); - assert (fold_case >= 0 && fold_case <= 2); { Bytecount dstlen; Index: src/vdb-posix.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/vdb-posix.c,v retrieving revision 1.2 diff -d -u -r1.2 vdb-posix.c --- src/vdb-posix.c 2006/03/27 15:20:31 1.2 +++ src/vdb-posix.c 2006/06/16 21:06:56 ＠＠ -73,7 +73,7 ＠＠ } else /* default sigsegv handler */ { - char *signal_name; + char *signal_name = ""; if (signum == SIGSEGV) signal_name = "SIGSEGV"; else if (signum == SIGBUS) Index: src/window.c =================================================================== RCS file: /pack/xemacscvs/XEmacs/xemacs/src/window.c,v retrieving revision 1.90 diff -d -u -r1.90 window.c --- src/window.c 2005/11/25 01:42:08 1.90 +++ src/window.c 2006/06/16 21:06:56 ＠＠ -4380,7 +4380,7 ＠＠ { int new_pixsize; sizep = &CURSIZE (w); - dim = CURCHARSIZE (w); + dim = window_char_width (w, 0); new_pixsize = inpixels?(*sizep + delta):(dim+delta); set_window_pixsize (window, new_pixsize, 0, 0); return; -- Jerry James, Assistant Professor james(a)xemacs.org Computer Science Department http://www.cs.usu.edu/~jerry/ Utah State University