GNOME Bugzilla – Bug 786995
Run analysis tools on GJS to prepare for release
Last modified: 2017-09-02 14:36:07 UTC
We have valgrind, cppcheck, asan, and ubsan easily usable now thanks to Claudio. Let's run each of these in preparation for the 1.50 release. cppcheck ======== [.../gjs/test/gjs-test-coverage.cpp:905]: (warning) sscanf() without field width limits can crash with huge input data. This is annoying to fix but in practice not a problem, because the maximum buffer size is computed. It could be fixed by using g_strdup_prinf("%%u,%%%zus", max_buffer_size) but then the compiler will complain about a non-string-literal format string. ubsan ===== Two patches attached.
Created attachment 358734 [details] [review] arg: Avoid assigning out-of-range values Assigning out-of-range values to variables of these types is undefined behaviour, and was caught by UBSan on Clang.
Created attachment 358735 [details] [review] object: Avoid taking address of empty vector Given a std::vector<T> vec, &vec[0] is undefined behaviour when the vector is empty. C++11 provides a vec.data() method which does the same thing safely. This is an opportunity for a small refactor; the free function didn't actually free the vector, so it should be renamed "clear", and it can just take in the vector instead of a pointer to the data. We can also keep all the memory management of the vector in the same place, in object_instance_init().
Created attachment 358797 [details] [review] build: Add LSan suppression file Compiling with ASan also automatically enables LSan (leak sanitizer). There is an intentional leak (of a few bytes) of GType structures in GJS, and SpiderMonkey also leaks a few bytes for each GC helper thread. Add a suppression file (unfortunately, a different format than Valgrind's suppression file) to make LSan ignore these leaks. One nice feature of LSan is that it prints a total of bytes leaked for each suppressed function.
Created attachment 358798 [details] [review] build: Don't use the vptr sanitizer SpiderMonkey compiles without runtime type information by default, and the vptr sanitizer (part of UBSan) is not compatible with that. This caused too many false positives to be useful.
Created attachment 358799 [details] [review] arg: Avoid taking abs() of -2^63 It's undefined behaviour to call abs() on (int64_t)(-2^63) because 2^63 can't be stored in an int64_t. Add a special case for this value.
Created attachment 358800 [details] [review] value: Fix memory leak while marshalling GValue g_value_set_boxed() copies the GValue, so if the GValue is holding a value that must be freed, e.g. a string, it needs to be unset. (g_value_take_boxed() will not work since the GValue that it would take ownership of is stack-allocated.)
Created attachment 358801 [details] [review] context: Avoid null pointer dereference gjs_context_eval_file() was returning false but not setting the error parameter if the file did not exist.
Created attachment 358802 [details] [review] build: Update Valgrind suppressions rules The suppressions rules were out of date. They can also be simplified a bit.
I still find two memory leaks with Valgrind, but without obvious solutions. I might open separate bug reports for those if I run out of time to investigate. I'll also try to get Valgrind integrated with the build system using AX_VALGRIND_CHECK [1]. [1] https://www.gnu.org/software/autoconf-archive/ax_valgrind_check.html
Review of attachment 358734 [details] [review]: OK
Created attachment 358811 [details] [review] build: Put dbus-run-session in AM_TESTS_ENVIRONMENT This makes the makefile simpler, and also fixes an inadvertently broken combination of code coverage plus running tests under Xvfb. This is needed for enabling Valgrind test runs with AX_VALGRIND_CHECK.
Created attachment 358812 [details] [review] build: Don't use LOG_COMPILER for shell scripts Give all the tests started with shell scripts their own FOO_LOG_COMPILER variable rather than using the common LOG_COMPILER one. This is because AX_VALGRIND_CHECK will override LOG_COMPILER with a valgrind command, and expect the test to be a binary, not a shell script. (Otherwise the shell binary will be run under valgrind, not the test.)
Created attachment 358813 [details] [review] build: Valgrind with AX_VALGRIND_CHECK This adds a "make check-valgrind" target which will run the test suite under four Valgrind tools in succession. Requires a bit of surgery on the shell script test runners, because AX_VALGRIND_CHECK mainly assumes your tests are binaries.
Created attachment 358884 [details] [review] build: Valgrind with AX_VALGRIND_CHECK This adds a "make check-valgrind" target which will run the test suite under two Valgrind tools in succession: memcheck and helgrind. Requires a bit of surgery on the shell script test runners, because AX_VALGRIND_CHECK mainly assumes your tests are binaries. The Valgrind tools drd and sgcheck are also available, but disabled by default because they seem to crash on SpiderMonkey's threading.
Review of attachment 358735 [details] [review]: ::: gi/object.cpp @@ +880,3 @@ * a hash) + * If returns false, then the GParameter elements in the passed-in vector must + * be unset by the caller. Perhaps I'm missing something, but aren't you calling the clear function also when this returns true below?
Review of attachment 358797 [details] [review]: OK
Review of attachment 358798 [details] [review]: OK
Review of attachment 358799 [details] [review]: OK
Review of attachment 358800 [details] [review]: Looks good.
Review of attachment 358801 [details] [review]: ::: gjs/context.cpp @@ +762,3 @@ ret = false; + g_set_error(error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND, + "File %s not found", filename); I would prefer to simply remove this check entirely; g_file_load_contents() should fail with the same error code already when the file does not exist.
Review of attachment 358802 [details] [review]: OK
Review of attachment 358811 [details] [review]: OK
Review of attachment 358812 [details] [review]: OK
Review of attachment 358884 [details] [review]: Nice, looks good to me.
Also, a test fails on Ubuntu 16.04 [1]. There are more to see, but I will wait for all patches to be in. - a new test? (process:30703): Gjs-WARNING **: JS ERROR: TypeError: Gtk.Widget.set_css_name is undefined defineGtkLegacyObjects/GtkWidgetClass<._init@resource:///org/gnome/gjs/modules/_legacy.js:677:17 wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22 defineGObjectLegacyObjects/GObjectMeta<._construct@resource:///org/gnome/gjs/modules/_legacy.js:543:13 wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22 Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/_legacy.js:117:20 Class@resource:///org/gnome/gjs/modules/_legacy.js:65:30 @/root/jhbuild/checkout/gjs/installed-tests/js/testLegacyGObject.js:782:30 JS CTX: Destroying JS context Bail out! JS_EvaluateScript() failed ERROR: installed-tests/js/testLegacyGObject.js - Bail out! JS_EvaluateScript() failed [1] https://travis-ci.org/claudioandre/gjs/builds/270577760
(In reply to Philip Chimento from comment #0) > > cppcheck > ======== > [.../gjs/test/gjs-test-coverage.cpp:905]: (warning) sscanf() without field > width limits can crash with huge input data. You can suppress warnings ([error id]:[filename]:[line]), if you wish. Anyway, isn't possible to use standard streams to replace this? PS: I can't run Coverity on GJS without authorization (their policy, so, worse than Travis), but I guess it will complain as cppcheck did: a lot of 'c'ism.
Created attachment 358897 [details] [review] tests: Suppress message about too-big integer In case of running the tests with fatal criticals, this should not abort the test, because it's expected.
Created attachment 358898 [details] [review] main: Use g_option_context_parse_strv() Using g_option_context_parse() on anything but the main function's argc and argv will leak memory. Instead, use g_option_context_parse_strv() which is intended for use with GStrv-style allocated arrays.
Created attachment 358899 [details] [review] cairo: Free popped pattern This pattern was leaked; according to the documentation the caller owns the return value of cairo_pop_group().
Review of attachment 358897 [details] [review]: OK
Review of attachment 358898 [details] [review]: Looks good.
Review of attachment 358899 [details] [review]: ::: modules/cairo-context.cpp @@ +721,1 @@ /* pattern belongs to the context, so keep the reference */ I think this comment needs to be updated/removed then.
(In reply to Cosimo Cecchi from comment #15) > Review of attachment 358735 [details] [review] [review]: > > ::: gi/object.cpp > @@ +880,3 @@ > * a hash) > + * If returns false, then the GParameter elements in the passed-in vector > must > + * be unset by the caller. > > Perhaps I'm missing something, but aren't you calling the clear function > also when this returns true below? You're right, it has to be cleared in both cases. I meant the comment in the sense that you might expect to clear it if the function returned true, but not if it returned false. But I see it's totally unclear, I'll fix it. > (AX_VALGRIND_CHECK patch) I got the AX_VALGRIND_DFLT macro slightly wrong and I'll attach another version of that patch, but I'll just go ahead and push the updated version.
Attachment 358734 [details] pushed as f88ab66 - arg: Avoid assigning out-of-range values Attachment 358797 [details] pushed as 3b8775f - build: Add LSan suppression file Attachment 358798 [details] pushed as 1eb8dc4 - build: Don't use the vptr sanitizer Attachment 358799 [details] pushed as 9a3e339 - arg: Avoid taking abs() of -2^63 Attachment 358800 [details] pushed as 61b1d8d - value: Fix memory leak while marshalling GValue Attachment 358811 [details] pushed as 531d33a - build: Put dbus-run-session in AM_TESTS_ENVIRONMENT Attachment 358812 [details] pushed as ded21f7 - build: Don't use LOG_COMPILER for shell scripts Attachment 358897 [details] pushed as c43fac7 - tests: Suppress message about too-big integer Attachment 358898 [details] pushed as d3d44e2 - main: Use g_option_context_parse_strv() Attachment 358899 [details] pushed as de56385 - cairo: Free popped pattern
Created attachment 358905 [details] [review] object: Avoid taking address of empty vector Given a std::vector<T> vec, &vec[0] is undefined behaviour when the vector is empty. C++11 provides a vec.data() method which does the same thing safely. This is an opportunity for a small refactor; the free function didn't actually free the vector, so it should be renamed "clear", and it can just take in the vector instead of a pointer to the data. We can also keep all the memory management of the vector in the same place, in object_instance_init().
Created attachment 358906 [details] [review] context: Avoid null pointer dereference gjs_context_eval_file() was returning false but not setting the error parameter if the file did not exist. That check makes no sense anyway, since it is also done by g_file_load_contents(). Therefore we remove it and take the opportunity to refactor the function to use autoptrs.
Created attachment 358907 [details] [review] build: Update Valgrind suppressions rules The suppressions rules were out of date. They can also be simplified a bit.
Created attachment 358908 [details] [review] build: Valgrind with AX_VALGRIND_CHECK This adds a "make check-valgrind" target which will run the test suite under two Valgrind tools in succession: memcheck and helgrind. Requires a bit of surgery on the shell script test runners, because AX_VALGRIND_CHECK mainly assumes your tests are binaries. The Valgrind tools drd and sgcheck are also available, but disabled by default because they seem to crash on SpiderMonkey's threading.
Created attachment 358909 [details] [review] maint: Add SpiderMonkey helgrind suppression rules Helgrind detects all kinds of data races in SpiderMonkey's atomic operations, but we can only assume that SpiderMonkey has got its atomic operations working correctly; these are not races in GJS, in any case.
(In reply to Claudio André from comment #25) > Also, a test fails on Ubuntu 16.04 [1]. There are more to see, but I will > wait for all patches to be in. > > - a new test? > > (process:30703): Gjs-WARNING **: JS ERROR: TypeError: > Gtk.Widget.set_css_name is undefined > defineGtkLegacyObjects/GtkWidgetClass<._init@resource:///org/gnome/gjs/ > modules/_legacy.js:677:17 > wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22 > defineGObjectLegacyObjects/GObjectMeta<._construct@resource:///org/gnome/gjs/ > modules/_legacy.js:543:13 > wrapper@resource:///org/gnome/gjs/modules/_legacy.js:82:22 > Class.prototype._construct/newClass@resource:///org/gnome/gjs/modules/ > _legacy.js:117:20 > Class@resource:///org/gnome/gjs/modules/_legacy.js:65:30 > @/root/jhbuild/checkout/gjs/installed-tests/js/testLegacyGObject.js:782:30 > > JS CTX: Destroying JS context > Bail out! JS_EvaluateScript() failed > ERROR: installed-tests/js/testLegacyGObject.js - Bail out! > JS_EvaluateScript() failed > > > [1] https://travis-ci.org/claudioandre/gjs/builds/270577760 That seems to indicate the test is running against GTK older than 3.20... is that the system GTK on 16.04? (In reply to Claudio André from comment #26) > (In reply to Philip Chimento from comment #0) > > > > cppcheck > > ======== > > [.../gjs/test/gjs-test-coverage.cpp:905]: (warning) sscanf() without field > > width limits can crash with huge input data. > > You can suppress warnings ([error id]:[filename]:[line]), if you wish. > Anyway, isn't possible to use standard streams to replace this? I do still intend to fix this, so I won't suppress it. > PS: I can't run Coverity on GJS without authorization (their policy, so, > worse than Travis), but I guess it will complain as cppcheck did: a lot of > 'c'ism. I have had this open for a long time, but it's been waiting on Coverity to review the project: https://scan.coverity.com/projects/gjs?tab=overview
Review of attachment 358905 [details] [review]: OK
Review of attachment 358906 [details] [review]: OK
Created attachment 358910 [details] [review] maint: Add SpiderMonkey helgrind suppression rules Helgrind detects all kinds of data races in SpiderMonkey's atomic operations, but we can only assume that SpiderMonkey has got its atomic operations working correctly; these are not races in GJS, in any case.
Review of attachment 358907 [details] [review]: I did not review the details of this change, but feel free to push.
Review of attachment 358910 [details] [review]: Ditto.
Attachment 358905 [details] pushed as ac42789 - object: Avoid taking address of empty vector Attachment 358906 [details] pushed as 51405b1 - context: Avoid null pointer dereference Attachment 358907 [details] pushed as 7e98865 - build: Update Valgrind suppressions rules Attachment 358908 [details] pushed as 73beb1e - build: Valgrind with AX_VALGRIND_CHECK Attachment 358910 [details] pushed as eba8ff7 - maint: Add SpiderMonkey helgrind suppression rules
(In reply to Philip Chimento from comment #40) > > ERROR: installed-tests/js/testLegacyGObject.js - Bail out! > > JS_EvaluateScript() failed > > > > > > [1] https://travis-ci.org/claudioandre/gjs/builds/270577760 > > That seems to indicate the test is running against GTK older than 3.20... is > that the system GTK on 16.04? Yes. It used to work, but I'm not able to find any `testLegacyGObject` on a successful ancient build. - I'm removing 16.04 as a valid build environment for GJS HEAD.
I figured out what's going on. GTK is an optional dependency, so with GTK 3.18 it just decides that you don't have GTK and doesn't compile the GTK module. However, testLegacyGObject includes some GTK stuff, so that needs to be conditional on the GTK dependency as well.
Ubuntu 16.04 ============================================================================ Testsuite summary for gjs 1.49.92 ============================================================================ # TOTAL: 895 # PASS: 881 # SKIP: 14 # XFAIL: 0 # FAIL: 0 # XPASS: 0 # ERROR: 0 ============================================================================ Fedora 26 and Ubuntu 17.04 ============================================================================ Testsuite summary for gjs 1.49.92 ============================================================================ # TOTAL: 909 # PASS: 895 # SKIP: 14 # XFAIL: 0a # FAIL: 0 # XPASS: 0 # ERROR: 0 ============================================================================