GNOME Bugzilla – Bug 715164
Clang static analysis fixes
Last modified: 2013-11-27 14:28:26 UTC
Various small fixes for bugs found by using scan-build on GTK+ (thanks to Murray for blogging about it: http://www.murrayc.com/permalink/2013/11/15/jhbuild-and-clangs-scan-build/).
Created attachment 261440 [details] [review] gbytes: Clarify the nullability of GBytes->data Clarify that it is permitted for a GBytes to contain a NULL data pointer, as long as its size is 0.
Created attachment 261441 [details] [review] gsubprocess: Fix potential strlen(NULL) calls Also clarify the nullability of stdin_buf arguments in GSubprocess communication calls. Found with scan-build.
Created attachment 261442 [details] [review] gvariant: Fix a potential memcpy(NULL) call This probably won’t crash, as it can only happen if (size == 0), but add a check to be safe, and to shut up the static analyser. This case can be reached with the following call: gvs_read_unaligned_le(NULL, 0) which can be called from: gvs_tuple_get_child(value, index_) with (value.data == NULL) and (value.size == 0). Found by scan-build.
Created attachment 261443 [details] [review] gfileutils: Fix a potential integer overflow When calculating the array sizes in get_contents_stdio(), there is a possibility of overflow for very large files. Rearrange the overflow checks to avoid this. The code already handled some possibilities of files being too large, so no new GError has been added to handle this; the existing G_FILE_ERROR_FAILED is re-used. Found by scan-build.
Review of attachment 261441 [details] [review]: Looks good, thank you!
Comment on attachment 261441 [details] [review] gsubprocess: Fix potential strlen(NULL) calls commit 299bcbfa41db0093bc876c1095f0aab248e3b771 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Mon Nov 25 13:35:53 2013 +0000 gsubprocess: Fix potential strlen(NULL) calls Also clarify the nullability of stdin_buf arguments in GSubprocess communication calls. Found with scan-build. https://bugzilla.gnome.org/show_bug.cgi?id=715164 gio/gsubprocess.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)
Created attachment 261453 [details] [review] gfileutils: Fix a potential integer overflow When calculating the array sizes in get_contents_stdio(), there is a possibility of overflow for very large files. Rearrange the overflow checks to avoid this. The code already handled some possibilities of files being too large, so no new GError has been added to handle this; the existing G_FILE_ERROR_FAILED is re-used. Found by scan-build.
Review of attachment 261442 [details] [review]: Ok.
Review of attachment 261453 [details] [review]: Anyone calling g_file_get_contents() on > 4GiB files on 32 bit is going to long ago have had their process OOM killed most likely, but it's nice the static analyzer caught this. Code looks right.
Review of attachment 261440 [details] [review]: ::: glib/gbytes.c @@ +85,3 @@ * + * @data is copied. It may be %NULL if @size is 0, to represent an empty array + * of bytes. Maybe just: Creates a new #GBytes by copying @data. If @size is 0, @data may be %NULL. The "represent an empty array of bytes" thing feels like we're beating them over the head a bit... @@ +117,3 @@ * g_bytes_new_with_free_func(). * + * @data may be %NULL if @size is 0, to represent an empty array of bytes. Ok, you have it repeated a few more times. Well, if you like it feel free to keep it. @@ +234,3 @@ * + * %NULL will be returned if @bytes is an empty array, in which case @size will + * be 0. One *could* do g_bytes_new_static ("", 0) in which case we'll return non-%NULL but bytes is an empty array. Ok, fairly pathological...but consider s/will be/may be/.
Thanks for the fast reviews, Colin. I realise there are a frustrating number of patches, but I couldn’t think of a better way to split them all up logically. Attachment 261442 [details] pushed as c1d5db6 - gvariant: Fix a potential memcpy(NULL) call Attachment 261453 [details] pushed as 33dd6d1 - gfileutils: Fix a potential integer overflow
Pushed with the suggested changes to not beat the poor user over the head, and also to briefly mention the pathological case from comment #10. Thanks Colin. Attachment 261440 [details] pushed as aa337d3 - gbytes: Clarify the nullability of GBytes->data
(In reply to comment #11) > Thanks for the fast reviews, Colin. I realise there are a frustrating number of > patches, Not at all! You increase your submitted patch count, I increase my reviewed count, we both win =)