{"thread":{"id":"31511","subject":"[PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption","startedAt":"2012-09-12T12:17:28Z","lastAt":"2012-09-13T17:46:46Z","messageCount":4,"participants":["Elia Pinto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"198829","messageId":"1347452248-12222-1-git-send-email-gitter.spiros@gmail.com","threadId":"31511","inReplyTo":null,"subject":"[PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2012-09-12T12:17:28Z","receivedAt":"2012-09-12T12:17:28Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Recent versions of Linux libc (later than 5.4.23) and glibc (2.x)\ninclude a malloc() implementation which is tunable via environment\nvariables. When MALLOC_CHECK_ is set, a special (less efficient)\nimplementation is used which is designed to be tolerant against\nsimple errors, such as double calls of free() with the same argument,\nor overruns of a single byte (off-by-one bugs). When MALLOC_CHECK_\nis set to 3, a diagnostic message is printed on stderr\nand the program is aborted.\n\nSetting the MALLOC_PERTURB_ environment variable causes the malloc\nfunctions in libc to return memory which has been wiped and clear\nmemory when it is returned.\nOf course this does not affect calloc which always does clear the memory.\n\nThe reason for this exercise is, of course, to find code which uses\nmemory returned by malloc without initializing it and code which uses\ncode after it is freed. valgrind can do this but it's costly to run.\nThe MALLOC_PERTURB_ exchanges the ability to detect problems in 100%\nof the cases with speed.\n\nThe byte value used to initialize values returned by malloc is the byte\nvalue of the environment value. The value used to clear memory is the\nbitwise inverse. Setting MALLOC_PERTURB_ to zero disables the feature.\n\nThis technique can find hard to detect bugs.\nIt is therefore suggested to always use this flag (at least temporarily)\nwhen testing out code or a new distribution.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n t/test-lib.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 78c4286..98c90b0 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -93,6 +93,12 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\n export EDITOR\n \n+# Add libc malloc_check and MALLOC_PERTURB test \n+export MALLOC_CHECK_=3\n+export MALLOC_PERTURB_=\"$( expr \\( $$ % 255 \\) + 1)\"\n+#\n+\n+\n # Protect ourselves from common misconfiguration to export\n # CDPATH into the environment\n unset CDPATH\n-- \n1.7.11.rc1\n"},{"id":"198860","messageId":"7vy5kfceod.fsf@alter.siamese.dyndns.org","threadId":"31511","inReplyTo":"1347452248-12222-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-12T17:51:46Z","receivedAt":"2012-09-12T17:51:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> Recent versions of Linux libc (later than 5.4.23) and glibc (2.x)\n> include a malloc() implementation which is tunable via environment\n> variables. When MALLOC_CHECK_ is set, a special (less efficient)\n> implementation is used which is designed to be tolerant against\n> simple errors, such as double calls of free() with the same argument,\n> or overruns of a single byte (off-by-one bugs). When MALLOC_CHECK_\n> is set to 3, a diagnostic message is printed on stderr\n> and the program is aborted.\n>\n> Setting the MALLOC_PERTURB_ environment variable causes the malloc\n> functions in libc to return memory which has been wiped and clear\n> memory when it is returned.\n> Of course this does not affect calloc which always does clear the memory.\n>\n> The reason for this exercise is, of course, to find code which uses\n> memory returned by malloc without initializing it and code which uses\n> code after it is freed. valgrind can do this but it's costly to run.\n> The MALLOC_PERTURB_ exchanges the ability to detect problems in 100%\n> of the cases with speed.\n>\n> The byte value used to initialize values returned by malloc is the byte\n> value of the environment value. The value used to clear memory is the\n> bitwise inverse. Setting MALLOC_PERTURB_ to zero disables the feature.\n>\n> This technique can find hard to detect bugs.\n> It is therefore suggested to always use this flag (at least temporarily)\n> when testing out code or a new distribution.\n>\n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n>  t/test-lib.sh | 6 ++++++\n>  1 file changed, 6 insertions(+)\n\nInteresting, but it bothers me to make it enabled unconditionally.\nAt least, this shouldn't be enabled under GIT_TEST_OPTS=--valgrind, no?\n\nBy the way, \"export VAR=VAL\" all on the same line, even though it is\nin POSIX.1, is reported to be unsupported by some shells people care\nabout, and needs to be corrected to \"VAR=VAL\" and \"export VAR\" as\nseparate commands.  I think we saw a patch to fix an instance or two\nthat snuck in recently.\n\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 78c4286..98c90b0 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -93,6 +93,12 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n>  export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\n>  export EDITOR\n>  \n> +# Add libc malloc_check and MALLOC_PERTURB test \n> +export MALLOC_CHECK_=3\n> +export MALLOC_PERTURB_=\"$( expr \\( $$ % 255 \\) + 1)\"\n> +#\n> +\n> +\n>  # Protect ourselves from common misconfiguration to export\n>  # CDPATH into the environment\n>  unset CDPATH\n"},{"id":"198935","messageId":"CA+EOSBmjwUA_Sogo5ZixnAE5_6C97wjEOzO6PnDgjw_BYzVz-w@mail.gmail.com","threadId":"31511","inReplyTo":"7vy5kfceod.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2012-09-13T16:36:53Z","receivedAt":"2012-09-13T16:36:53Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2012/9/12 Junio C Hamano <gitster@pobox.com>:\n>\n> Interesting, but it bothers me to make it enabled unconditionally.\n> At least, this shouldn't be enabled under GIT_TEST_OPTS=--valgrind, no?\nSorry for the late response and thanks.\n\nNo, setting MALLOC_CHECK don't require\nvalgrind and it considered a best QA to have the test suite with it\ndefined always. If the test suite fail with MALLOC_CHECK, well, there\nis some problem, no ?\nSome distro do it already in building packages (fedora for example)\nhttp://emacs.1067599.n5.nabble.com/please-set-both-MALLOC-PERTURB-and-MALLOC-CHECK-envvars-td150144.html\n\nAt @rpm5.org we do the same for popt, for example, from years\n\nhttp://rpm5.org/community/rpm-devel/4156.html\n>\n> By the way, \"export VAR=VAL\" all on the same line, even though it is\n> in POSIX.1, is reported to be unsupported by some shells people care\n> about, and needs to be corrected to \"VAR=VAL\" and \"export VAR\" as\n> separate commands.  I think we saw a patch to fix an instance or two\n> that snuck in recently.\nYes, right, my bad. I will reroll.\n\nThank you\n"},{"id":"198941","messageId":"7vvcfh95o9.fsf@alter.siamese.dyndns.org","threadId":"31511","inReplyTo":"CA+EOSBmjwUA_Sogo5ZixnAE5_6C97wjEOzO6PnDgjw_BYzVz-w@mail.gmail.com","subject":"Re: [PATCH] Add MALLOC_CHECK_ and MALLOC_PERTURB_ libc env to the test suite for detecting heap corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-13T17:46:46Z","receivedAt":"2012-09-13T17:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> 2012/9/12 Junio C Hamano <gitster@pobox.com>:\n>>\n>> Interesting, but it bothers me to make it enabled unconditionally.\n>> At least, this shouldn't be enabled under GIT_TEST_OPTS=--valgrind, no?\n> Sorry for the late response and thanks.\n>\n> No, setting MALLOC_CHECK don't require\n> valgrind ...\n\nYou never said anything like that, and I didn't question it.\n\n> and it considered a best QA to have the test suite with it\n> defined always. If the test suite fail with MALLOC_CHECK, well, there\n> is some problem, no ?\n\nI never said using MALLOC_CHECK_ is a bad idea.\n\nLet me ask the same question in a different way, as I seem to have\nfailed in the previous message.\n\nIf you are using valgrind to run tests, is it sane to also enable\nMALLOC_CHECK?  If you were testing \"cat\", would it make sense to do:\n\n\t$ MALLOC_CHECK_=3 valgrind cat README\n\nBecause we are not interested in testing how valgrind (not cat)\nuses malloc, we may be better off running\n\n\t$ valgrind cat README\n\nwithout MALLOC_CHECK_; it will reduce the risk of MALLOC_CHECK_\npotentially disturbing what we really want to check (i.e. cat) by\ntriggering for something whose problems we are not trying to see\n(i.e. valgrind), no?\n\nThat was my question.\n"}]}