{"thread":{"id":"46704","subject":"[PATCH 0/10] towards clean leak-checker output","startedAt":"2017-09-05T13:02:00Z","lastAt":"2017-09-20T05:12:43Z","messageCount":42,"participants":["Jeff King","Martin Ågren","Stefan Beller","Junio C Hamano","Johannes Schindelin","Brandon Williams","Kaartic Sivaraam","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"327569","messageId":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","threadId":"46704","inReplyTo":null,"subject":"[PATCH 0/10] towards clean leak-checker output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:01:50Z","receivedAt":"2017-09-05T13:02:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using leak-checking tools like valgrind or LSAN is usually not all that\nproductive with Git, because the output is far from clean. And _most_ of\nthese are just not interesting, as they're either:\n\n 1. Real leaks, but ones that only be triggered a small, set number of\n times per program.\n\n 2. Intentional leaks of variables as the main cmd_* functions go out\n of scope (as opposed to manually cleaning).\n\nI focused here on getting t0000 and t0001 to run clean against a\nleak-checking tool. That's a fraction of the total test suite, but my\ngoal was have a tractable sample which could let us see if the path\nseems sane.\n\nI feel positive overall about the approach this series takes. The\nsuppressions aren't too terrible to write, and I found some real (albeit\nminor) leaks in these few tests. I hope it can serve as a base for an\nongoing effort to get the whole test suite clean.\n\nA few things to note:\n\n - I switched from valgrind to ASAN/LSAN early on. It's just way\n faster, which makes the compile-test-fix cycle a lot more pleasant.\n With these patches, you should be able to do:\n\n make SANITIZE=leak && (cd t && ./t1234-* -v -i)\n\n and get a leak report for a single script dumped to stderr.\n\n If you want to try it with t0000, you'll need the lock-file series I\n just posted at:\n\n https://public-inbox.org/git/20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net/\n\n and the fix from Martin at:\n\n https://public-inbox.org/git/CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com/\n\n to get a clean run.\n\n - I'm using LSAN instead of the full-on ASAN because it's faster. The\n docs warn that it's a bit experimental, and I did notice a few funny\n behaviors (like truncated backtraces), but overall it seems fine.\n You can also do:\n\n make SANITIZE=address &&\n (cd t && ASAN_OPTIONS=abort_on_error=1 ./t1234-* -v -i)\n\n to do a full ASAN run (the extra environment setting is necessary to\n override test-lib's defaults).\n\n - gcc's leak-checker doesn't seem to handle reachability correctly (or\n maybe I'm holding it wrong). As a simple case, building with ASAN\n and running git-init complains:\n\n $ make SANITIZE=address && ./git init foo\n ...\n Direct leak of 2 byte(s) in 1 object(s) allocated from:\n #0 0x7f011dc699e0 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd99e0)\n #1 0x558eeedbdab5 in do_xmalloc /home/peff/compile/git/wrapper.c:60\n #2 0x558eeedbdbab in do_xmallocz /home/peff/compile/git/wrapper.c:100\n #3 0x558eeedbdd0d in xmallocz /home/peff/compile/git/wrapper.c:108\n #4 0x558eeedbdd0d in xmemdupz /home/peff/compile/git/wrapper.c:124\n #5 0x558eeedbdd0d in xstrndup /home/peff/compile/git/wrapper.c:130\n #6 0x558eeea0507a in main /home/peff/compile/git/common-main.c:39\n #7 0x7f011cf612e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)\n\n That line is the setting of argv0_path, which is a global (and thus\n shouldn't be marked as leaking). Interestingly, it only happens with\n -O2. Compiling with -O0 works fine. I'm not sure if it's a bug or\n what.\n\n I did most of my testing with clang-6.0, which gets this case right.\n\n - I have no idea who close or far this is to covering the whole suite.\n Only 98 test scripts complete with no leaks. But many of those\n failures will be hitting the same leaks over and over. It looks like\n running \"git show' hits a few, which is going to affect a lot of\n scripts. But bear in mind when reading the patches that this might\n be the first step on a successful road, or it could be a dead end. :)\n\nMost of this is actual leak fixes. The interesting part, I think, is the\nUNLEAK annotation in patch 10.\n\n [01/10]: test-lib: --valgrind should not override --verbose-log\n [02/10]: test-lib: set LSAN_OPTIONS to abort by default\n [03/10]: add: free leaked pathspec after add_files_to_cache()\n [04/10]: update-index: fix cache entry leak in add_one_file()\n [05/10]: config: plug user_config leak\n [06/10]: reset: make tree counting less confusing\n [07/10]: reset: free allocated tree buffers\n [08/10]: repository: free fields before overwriting them\n [09/10]: set_git_dir: handle feeding gitdir to itself\n [10/10]: add UNLEAK annotation for reducing leak false positives\n\n Makefile | 3 +++\n builtin/add.c | 3 +++\n builtin/commit.c | 1 +\n builtin/config.c | 11 +++++++++--\n builtin/init-db.c | 2 ++\n builtin/ls-files.c | 1 +\n builtin/reset.c | 24 +++++++++++++++++-------\n builtin/update-index.c | 4 +++-\n builtin/worktree.c | 2 ++\n environment.c | 4 +++-\n git-compat-util.h | 7 +++++++\n repository.c | 14 +++++++-------\n setup.c | 5 -----\n t/test-lib.sh | 7 ++++++-\n usage.c | 13 +++++++++++++\n 15 files changed, 77 insertions(+), 24 deletions(-)\n\n-Peff\n"},{"id":"327570","messageId":"20170905130354.ydaasossmjlaps22@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 01/10] test-lib: --valgrind should not override --verbose-log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:03:54Z","receivedAt":"2017-09-05T13:04:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The --verbose test option cannot be used with test harnesses\nlike \"prove\". Instead, you must use --verbose-log.\n\nSince the --valgrind option implies --verbose, that means\nthat it cannot be used with prove. I.e., this does not work:\n\n prove t0000-basic.sh :: --valgrind\n\nYou'd think it could be fixed by doing:\n\n prove t0000-basic.sh :: --valgrind --verbose-log\n\nbut that doesn't work either, because the implied --verbose\ntakes precedence over --verbose-log. If the user has given\nus a specific option, we should prefer that.\n\nSigned-off-by: Jeff King \n---\nI ended up not using valgrind for most of my tests, but this did bite me\nearly on when I tried to run \"make GIT_TEST_OPTS=--valgrind prove\".\n\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 5fbd8d4a90..62461a6e35 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -274,7 +274,7 @@ then\n \ttest -z \"$verbose\" && verbose_only=\"$valgrind_only\"\n elif test -n \"$valgrind\"\n then\n-\tverbose=t\n+\ttest -z \"$verbose_log\" && verbose=t\n fi\n \n if test -n \"$color\"\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327571","messageId":"20170905130404.isih63vmyyxx5aqp@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 02/10] test-lib: set LSAN_OPTIONS to abort by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:04Z","receivedAt":"2017-09-05T13:04:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already set ASAN_OPTIONS to abort if it finds any errors.\nAs we start to experiment with LSAN, the leak sanitizer,\nit's convenient if we give it the same treatment.\n\nNote that ASAN is actually a superset of LSAN and can do the\nleak detection itself. So this only has an effect if you\nspecifically build with \"make SANITIZE=leak\" (leak detection\nbut not the rest of ASAN). Building with just LSAN results\nin a build that runs much faster. That makes the\nbuild-test-fix cycle more pleasant.\n\nIn the long run, once we've fixed or suppressed all the\nleaks, it will probably be worth turning leak-detection on\nfor ASAN and just using that (to check both leaks _and_\nmemory errors in a single test run). But there's still a lot\nof work before we get there.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 62461a6e35..a738540ef2 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -44,6 +44,11 @@ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n : ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n export ASAN_OPTIONS\n \n+# If LSAN is in effect we _do_ want leak checking, but we still\n+# want to abort so that we notice the problems.\n+: ${LSAN_OPTIONS=abort_on_error=1}\n+export LSAN_OPTIONS\n+\n ################################################################\n # It appears that people try to run tests without building...\n \"$GIT_BUILD_DIR/git\" >/dev/null\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327572","messageId":"20170905130410.3sexuoc7a42yc2t7@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 03/10] add: free leaked pathspec after add_files_to_cache()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:10Z","receivedAt":"2017-09-05T13:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After run_diff_files, we throw away the rev_info struct,\nincluding the pathspec that we copied into it, leaking the\nmemory. this is probably not a big deal in practice. We\nusually only run this once per process, and the leak is\nproportional to the pathspec list we're already holding in\nmemory.\n\nBut it's still a leak, and it pollutes leak-checker output,\nmaking it harder to find important leaks.\n\nSigned-off-by: Jeff King \n---\n builtin/add.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c20548e4f5..ef625e3fb8 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -119,6 +119,7 @@ int add_files_to_cache(const char *prefix,\n \trev.diffopt.flags |= DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG;\n \trev.max_count = 0; /* do not compare unmerged paths with stage #2 */\n \trun_diff_files(&rev, DIFF_RACY_IS_MODIFIED);\n+\tclear_pathspec(&rev.prune_data);\n \treturn !!data.add_errors;\n }\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327573","messageId":"20170905130413.eyifiklpr53s5kg4@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 04/10] update-index: fix cache entry leak in add_one_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:14Z","receivedAt":"2017-09-05T13:04:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fail to add the cache entry to the index, we end up\njust leaking the struct. We should follow the pattern of the\nearly-return above and free it.\n\nSigned-off-by: Jeff King \n---\n builtin/update-index.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 655e6d60d3..bf7420b808 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -287,8 +287,10 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len\n \t}\n \toption = allow_add ? ADD_CACHE_OK_TO_ADD : 0;\n \toption |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;\n-\tif (add_cache_entry(ce, option))\n+\tif (add_cache_entry(ce, option)) {\n+\t\tfree(ce);\n \t\treturn error(\"%s: cannot add to the index - missing --add option?\", path);\n+\t}\n \treturn 0;\n }\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327574","messageId":"20170905130419.q27w6nblsryegldg@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 05/10] config: plug user_config leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:20Z","receivedAt":"2017-09-05T13:04:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We generate filenames for the user_config (\"~/.gitconfig\")\nand the xdg config (\"$XDG_CONFIG_HOME/git/config\") and then\ndecide which to use by looking at the filesystem. But after\nselecting one, the unused string is just leaked.\n\nThis is a tiny leak, but it creates noise in leak-checker\noutput.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 70ff231e9c..52a4606243 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -518,10 +518,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"$HOME not set\");\n \n \t\tif (access_or_warn(user_config, R_OK, 0) &&\n-\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0))\n+\t\t xdg_config && !access_or_warn(xdg_config, R_OK, 0)) {\n \t\t\tgiven_config_source.file = xdg_config;\n-\t\telse\n+\t\t\tfree(user_config);\n+\t\t} else {\n \t\t\tgiven_config_source.file = user_config;\n+\t\t\tfree(xdg_config);\n+\t\t}\n \t}\n \telse if (use_system_config)\n \t\tgiven_config_source.file = git_etc_gitconfig();\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327575","messageId":"20170905130428.pxvhvlqcprefgzgc@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 06/10] reset: make tree counting less confusing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:28Z","receivedAt":"2017-09-05T13:04:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Depending on whether we're in --keep mode, git-reset may\nfeed one or two trees to unpack_trees(). We start a counter\nat \"1\" and then increment it to \"2\" only for the two-tree\ncase. But that means we must always subtract one to find the\ncorrect array slot to fill with each descriptor.\n\nInstead, let's start at \"0\" and just increment our counter\nafter adding each tree. This skips the extra subtraction,\nand will make things much easier when we start to actually\nfree our tree buffers.\n\nWhile we're at it, let's make the first allocation use the\nslot at \"desc + nr\", too, even though we know \"nr\" is 0 at\nthat point. It makes the two fill_tree_descriptor() calls\nconsistent (always \"desc + nr\", followed by always\nincrementing \"nr\").\n\nSigned-off-by: Jeff King \n---\n builtin/reset.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex f1af9345e4..34839db8ca 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -44,7 +44,7 @@ static inline int is_merge(void)\n \n static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n {\n-\tint nr = 1;\n+\tint nr = 0;\n \tstruct tree_desc desc[2];\n \tstruct tree *tree;\n \tstruct unpack_trees_options opts;\n@@ -75,14 +75,16 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n \t\tstruct object_id head_oid;\n \t\tif (get_oid(\"HEAD\", &head_oid))\n \t\t\treturn error(_(\"You do not have a valid HEAD.\"));\n-\t\tif (!fill_tree_descriptor(desc, &head_oid))\n+\t\tif (!fill_tree_descriptor(desc + nr, &head_oid))\n \t\t\treturn error(_(\"Failed to find tree of HEAD.\"));\n \t\tnr++;\n \t\topts.fn = twoway_merge;\n \t}\n \n-\tif (!fill_tree_descriptor(desc + nr - 1, oid))\n+\tif (!fill_tree_descriptor(desc + nr, oid))\n \t\treturn error(_(\"Failed to find tree of %s.\"), oid_to_hex(oid));\n+\tnr++;\n+\n \tif (unpack_trees(nr, desc, &opts))\n \t\treturn -1;\n \n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327576","messageId":"20170905130450.fsoglfwieei2ruir@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 07/10] reset: free allocated tree buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:51Z","receivedAt":"2017-09-05T13:05:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We read the tree objects with fill_tree_descriptor(), but\nnever actually free the resulting buffers, causing a memory\nleak. This isn't a huge deal because we call this code at\nmost twice per program invocation. But it does potentially\ndouble our heap usage if you have large root trees. Let's\nfree the trees before returning.\n\n\nSigned-off-by: Jeff King \n---\nActually, in the two-call case we repeat one of the trees,\nso we _could_ in theory stash it away. But we don't do that\ncurrently, and it's probably not worth the effort. My main\nmotivation is just shutting up the leak checker so I can\nfind actual important leaks.\n\n builtin/reset.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 34839db8ca..9e22b4a1dc 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -44,10 +44,11 @@ static inline int is_merge(void)\n \n static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n {\n-\tint nr = 0;\n+\tint i, nr = 0;\n \tstruct tree_desc desc[2];\n \tstruct tree *tree;\n \tstruct unpack_trees_options opts;\n+\tint ret = -1;\n \n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = 1;\n@@ -81,19 +82,26 @@ static int reset_index(const struct object_id *oid, int reset_type, int quiet)\n \t\topts.fn = twoway_merge;\n \t}\n \n-\tif (!fill_tree_descriptor(desc + nr, oid))\n-\t\treturn error(_(\"Failed to find tree of %s.\"), oid_to_hex(oid));\n+\tif (!fill_tree_descriptor(desc + nr, oid)) {\n+\t\terror(_(\"Failed to find tree of %s.\"), oid_to_hex(oid));\n+\t\tgoto out;\n+\t}\n \tnr++;\n \n-\tif (unpack_trees(nr, desc, &opts))\n-\t\treturn -1;\n+\tif (unpack_trees(nr, desc, &opts)) \n+\t\tgoto out;\n \n \tif (reset_type == MIXED || reset_type == HARD) {\n \t\ttree = parse_tree_indirect(oid);\n \t\tprime_cache_tree(&the_index, tree);\n \t}\n \n-\treturn 0;\n+\tret = 0;\n+\n+out:\n+\tfor (i = 0; i < nr; i++)\n+\t\tfree((void *)desc[i].buffer);\n+\treturn ret;\n }\n \n static void print_new_head_line(struct commit *commit)\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327577","messageId":"20170905130456.ecf72u4u4mbljavo@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 08/10] repository: free fields before overwriting them","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:04:57Z","receivedAt":"2017-09-05T13:05:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's possible that the repository data may be initialized\ntwice (e.g., after doing a chdir() to the top of the\nworktree we may have to adjust a relative git_dir path). We\nshould free() any existing fields before assigning to them\nto avoid leaks.\n\nThis should be safe, as the fields are set based on the\nenvironment or on other strings like the gitdir or\ncommondir. That makes it impossible that we are feeding an\nalias to the just-freed string.\n\nSigned-off-by: Jeff King \n---\n environment.c | 4 +++-\n repository.c | 4 ++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/environment.c b/environment.c\nindex 3fd4b10845..f1f934b6fd 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -97,7 +97,7 @@ int ignore_untracked_cache_config;\n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n char *git_work_tree_cfg;\n \n-static const char *namespace;\n+static char *namespace;\n \n static const char *super_prefix;\n \n@@ -152,8 +152,10 @@ void setup_git_env(void)\n \tif (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))\n \t\tcheck_replace_refs = 0;\n \treplace_ref_base = getenv(GIT_REPLACE_REF_BASE_ENVIRONMENT);\n+\tfree(git_replace_ref_base);\n \tgit_replace_ref_base = xstrdup(replace_ref_base ? replace_ref_base\n \t\t\t\t\t\t\t : \"refs/replace/\");\n+\tfree(namespace);\n \tnamespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));\n \tshallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);\n \tif (shallow_file)\ndiff --git a/repository.c b/repository.c\nindex f107af7d76..52f1821c6b 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -40,11 +40,15 @@ static void repo_setup_env(struct repository *repo)\n \n \trepo->different_commondir = find_common_dir(&sb, repo->gitdir,\n \t\t\t\t\t\t !repo->ignore_env);\n+\tfree(repo->commondir);\n \trepo->commondir = strbuf_detach(&sb, NULL);\n+\tfree(repo->objectdir);\n \trepo->objectdir = git_path_from_env(DB_ENVIRONMENT, repo->commondir,\n \t\t\t\t\t \"objects\", !repo->ignore_env);\n+\tfree(repo->graft_file);\n \trepo->graft_file = git_path_from_env(GRAFT_ENVIRONMENT, repo->commondir,\n \t\t\t\t\t \"info/grafts\", !repo->ignore_env);\n+\tfree(repo->index_file);\n \trepo->index_file = git_path_from_env(INDEX_ENVIRONMENT, repo->gitdir,\n \t\t\t\t\t \"index\", !repo->ignore_env);\n }\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327578","messageId":"20170905130501.xwc2ud2pwpbhqsli@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 09/10] set_git_dir: handle feeding gitdir to itself","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:05:01Z","receivedAt":"2017-09-05T13:05:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Ideally we'd free the existing gitdir field before assigning\nthe new one, to avoid a memory leak. But we can't do so\nsafely because some callers do the equivalent of:\n\n set_git_dir(get_git_dir());\n\nWe can detect that case as a noop, but there are even more\ncomplicated cases like:\n\n set_git_dir(remove_leading_path(worktree, get_git_dir());\n\nwhere we really do need to do some work, but the original\nstring must remain valid.\n\nRather than put the burden on callers to make a copy of the\nstring (only to free it later, since we'll make a copy of it\nourselves), let's solve the problem inside set_git_dir(). We\ncan make a copy of the pointer for the old gitdir, and then\navoid freeing it until after we've made our new copy.\n\nSigned-off-by: Jeff King \n---\n repository.c | 10 +++-------\n setup.c | 5 -----\n 2 files changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/repository.c b/repository.c\nindex 52f1821c6b..97c732bd48 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -56,16 +56,12 @@ static void repo_setup_env(struct repository *repo)\n void repo_set_gitdir(struct repository *repo, const char *path)\n {\n \tconst char *gitfile = read_gitfile(path);\n+\tchar *old_gitdir = repo->gitdir;\n \n-\t/*\n-\t * NEEDSWORK: Eventually we want to be able to free gitdir and the rest\n-\t * of the environment before reinitializing it again, but we have some\n-\t * crazy code paths where we try to set gitdir with the current gitdir\n-\t * and we don't want to free gitdir before copying the passed in value.\n-\t */\n \trepo->gitdir = xstrdup(gitfile ? gitfile : path);\n-\n \trepo_setup_env(repo);\n+\n+\tfree(old_gitdir);\n }\n \n /*\ndiff --git a/setup.c b/setup.c\nindex 23950173fc..6d8380acd2 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -399,11 +399,6 @@ void setup_work_tree(void)\n \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n \n-\t/*\n-\t * NEEDSWORK: this call can essentially be set_git_dir(get_git_dir())\n-\t * which can cause some problems when trying to free the old value of\n-\t * gitdir.\n-\t */\n \tset_git_dir(remove_leading_path(git_dir, work_tree));\n \tinitialized = 1;\n }\n-- \n2.14.1.721.gc5bc1565f1\n\n"},{"id":"327579","messageId":"20170905130505.him3p4jhxp64r2vy@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T13:05:06Z","receivedAt":"2017-09-05T13:05:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's a common pattern in git commands to allocate some\nmemory that should last for the lifetime of the program and\nthen not bother to free it, relying on the OS to throw it\naway.\n\nThis keeps the code simple, and it's fast (we don't waste\ntime traversing structures or calling free at the end of the\nprogram). But it also triggers warnings from memory-leak\ncheckers like valgrind or LSAN. They know that the memory\nwas still allocated at program exit, but they don't know\n_when_ the leaked memory stopped being useful. If it was\nearly in the program, then it's probably a real and\nimportant leak. But if it was used right up until program\nexit, it's not an interesting leak and we'd like to suppress\nit so that we can see the real leaks.\n\nThis patch introduces an UNLEAK() macro that lets us do so.\nTo understand its design, let's first look at some of the\nalternatives.\n\nUnfortunately the suppression systems offered by\nleak-checking tools don't quite do what we want. A\nleak-checker basically knows two things:\n\n 1. Which blocks were allocated via malloc, and the\n callstack during the allocation.\n\n 2. Which blocks were left un-freed at the end of the\n program (and which are unreachable, but more on that\n later).\n\nTheir suppressions work by mentioning the function or\ncallstack of a particular allocation, and marking it as OK\nto leak. So imagine you have code like this:\n\n int main(void)\n {\n\t/* this allocates some memory */\n\tchar *p = some_function();\n\tprintf(\"%s\", p);\n\treturn 0;\n }\n\nYou can say \"ignore allocations from some_function(),\nthey're not leaks\". But that's not right. That function may\nbe called elsewhere, too, and we would potentially want to\nknow about those leaks.\n\nSo you can say \"ignore the callstack when main calls\nsome_function\". That works, but your annotations are\nbrittle. In this case it's only two functions, but you can\nimagine that the actual allocation is much deeper. If any of\nthe intermediate code changes, you have to update the\nsuppression.\n\nWhat we _really_ want to say is that \"the value assigned to\np at the end of the function is not a real leak\". But\nleak-checkers can't understand that; they don't know about\n\"p\" in the first place.\n\nHowever, we can do something a little bit tricky if we make\nsome assumptions about how leak-checkers work. They\ngenerally don't just report all un-freed blocks. That would\nreport even globals which are still accessible when the\nleak-check is run. Instead they take some set of memory\n(like BSS) as a root and mark it as \"reachable\". Then they\nscan the reachable blocks for anything that looks like a\npointer to a malloc'd block, and consider that block\nreachable. And then they scan those blocks, and so on,\ntransitively marking anything reachable from a global as\n\"not leaked\" (or at least leaked in a different category).\n\nSo we can mark the value of \"p\" as reachable by putting it\ninto a variable with program lifetime. One way to do that is\nto just mark \"p\" as static. But that actually affects the\nrun-time behavior if the function is called twice (you\naren't likely to call main() twice, but some of our cmd_*()\nfunctions are called from other commands).\n\nInstead, we can trick the leak-checker by putting the value\ninto _any_ reachable bytes. This patch keeps a global\nlinked-list of bytes copied from \"unleaked\" variables. That\nlist is reachable even at program exit, which confers\nrecursive reachability on whatever values we unleak.\n\nIn other words, you can do:\n\n int main(void)\n {\n\tchar *p = some_function();\n\tprintf(\"%s\", p);\n\tUNLEAK(p);\n\treturn 0;\n }\n\nto annotate \"p\" and suppress the leak report.\n\nBut wait, couldn't we just say \"free(p)\"? In this toy\nexample, yes. But using UNLEAK() has several advantages over\nactually freeing the memory:\n\n 1. It can be compiled conditionally. There's no need in\n normal runs to do this free(), and it just wastes time.\n By using a macro, we can get the benefit for leak-check\n builds with zero cost for normal builds (this patch\n uses a compile-time check, though we could clearly also\n make it a run-time check at very low cost).\n\n Of course one could also hide free() behind a macro, so\n this is really just arguing for having UNLEAK(), not\n for its particular implementation.\n\n 2. It's recursive across structures. In many cases our \"p\"\n is not just a pointer, but a complex struct whose\n fields may have been allocated by a sub-function. And\n in some cases (e.g., dir_struct) we don't even have a\n function which knows how to free all of the struct\n members.\n\n By marking the struct itself as reachable, that confers\n reachability on any pointers it contains (including those\n found in embedded structs, or reachable by walking\n heap blocks recursively.\n\n 3. It works on cases where we're not sure if the value is\n allocated or not. For example:\n\n char *p = argc > 1 ? argv[1] : some_function();\n\n It's safe to use UNLEAK(p) here, because it's not\n freeing any memory. In the case that we're pointing to\n argv here, the reachability checker will just ignore\n our bytes.\n\n 4. Because it's not actually freeing memory, you can\n UNLEAK() before we are finished accessing the variable.\n This is helpful in cases like this:\n\n char *p = some_function();\n return another_function(p);\n\n Writing this with free() requires:\n\n int ret;\n char *p = some_function();\n ret = another_function(p);\n free(p);\n return ret;\n\n But with unleak we can just write:\n\n char *p = some_function();\n UNLEAK(p);\n return another_function(p);\n\nThis patch adds the UNLEAK() macro and enables it\nautomatically when Git is compiled with SANITIZE=leak.\nIt adds some UNLEAK() annotations to show off how the\nfeature works. On top of other recent leak fixes, these are\nenough to get t0000 and t0001 to pass when compiled with\nLSAN.\n\nSigned-off-by: Jeff King \n---\n Makefile | 3 +++\n builtin/add.c | 2 ++\n builtin/commit.c | 1 +\n builtin/config.c | 4 ++++\n builtin/init-db.c | 2 ++\n builtin/ls-files.c | 1 +\n builtin/worktree.c | 2 ++\n git-compat-util.h | 7 +++++++\n usage.c | 13 +++++++++++++\n 9 files changed, 35 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex f2bb7f2f63..c052f09bba 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1036,6 +1036,9 @@ BASIC_CFLAGS += -fno-omit-frame-pointer\n ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n endif\n+ifneq ($(filter leak,$(SANITIZERS)),)\n+BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n+endif\n endif\n \n ifndef sysconfdir\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ef625e3fb8..a648cf4c56 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -515,5 +515,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Unable to write new index file\"));\n \t}\n \n+\tUNLEAK(pathspec);\n+\tUNLEAK(dir);\n \treturn exit_status;\n }\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3b04f5dd3..de775d906c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1819,5 +1819,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tprint_summary(prefix, &oid, !current_head);\n \n \tstrbuf_release(&err);\n+\tUNLEAK(sb);\n \treturn 0;\n }\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 52a4606243..d13daeeb55 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -631,6 +631,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(_(\"cannot overwrite multiple values with a single value\\n\"\n@@ -641,6 +642,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2], 0);\n \t}\n@@ -648,6 +650,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value,\n \t\t\t\t\t\t\t CONFIG_REGEX_NONE, 0);\n@@ -656,6 +659,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2], 1);\n \t}\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 47823f9aa4..c9b7946bad 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -579,6 +579,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tset_git_work_tree(work_tree);\n \t}\n \n+\tUNLEAK(real_git_dir);\n+\n \tflags |= INIT_DB_EXIST_OK;\n \treturn init_db(git_dir, real_git_dir, template_dir, flags);\n }\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex e1339e6d17..8c713c47ac 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -673,5 +673,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\treturn bad ? 1 : 0;\n \t}\n \n+\tUNLEAK(dir);\n \treturn 0;\n }\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex c98e2ce5f5..de26849f55 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -381,6 +381,8 @@ static int add(int ac, const char **av, const char *prefix)\n \t\tbranch = opts.new_branch;\n \t}\n \n+\tUNLEAK(path);\n+\tUNLEAK(opts);\n \treturn add_worktree(path, branch, &opts);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 6678b488cc..01cde2e375 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1169,4 +1169,11 @@ static inline int is_missing_file_error(int errno_)\n \n extern int cmd_main(int, const char **);\n \n+#ifdef SUPPRESS_ANNOTATED_LEAKS\n+extern void unleak_memory(const void *ptr, size_t len);\n+#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n+#else\n+#define UNLEAK(var)\n+#endif\n+\n #endif\ndiff --git a/usage.c b/usage.c\nindex 1ea7df9a20..780ed73be6 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -241,3 +241,16 @@ NORETURN void BUG(const char *fmt, ...)\n \tva_end(ap);\n }\n #endif\n+\n+void unleak_memory(const void *ptr, size_t len)\n+{\n+\tstatic struct suppressed_leak_root {\n+\t\tstruct suppressed_leak_root *next;\n+\t\tchar data[FLEX_ARRAY];\n+\t} *suppressed_leaks;\n+\tstruct suppressed_leak_root *root;\n+\n+\tFLEX_ALLOC_MEM(root, data, ptr, len);\n+\troot->next = suppressed_leaks;\n+\tsuppressed_leaks = root;\n+}\n-- \n2.14.1.721.gc5bc1565f1\n"},{"id":"327594","messageId":"CAN0heSrVzRPc+iVqU02qzk=DB0WT6Fscn6X-hZPFkM1TikMPVQ@mail.gmail.com","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] towards clean leak-checker output","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-05T17:50:10Z","receivedAt":"2017-09-05T17:50:20Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 15:01, Jeff King wrote:\n> Using leak-checking tools like valgrind or LSAN is usually not all that\n> productive with Git, because the output is far from clean. And _most_ of\n> these are just not interesting, as they're either:\n>\n> 1. Real leaks, but ones that only be triggered a small, set number of\n> times per program.\n>\n> 2. Intentional leaks of variables as the main cmd_* functions go out\n> of scope (as opposed to manually cleaning).\n>\n> I focused here on getting t0000 and t0001 to run clean against a\n> leak-checking tool. That's a fraction of the total test suite, but my\n> goal was have a tractable sample which could let us see if the path\n> seems sane.\n>\n> I feel positive overall about the approach this series takes. The\n> suppressions aren't too terrible to write, and I found some real (albeit\n> minor) leaks in these few tests. I hope it can serve as a base for an\n> ongoing effort to get the whole test suite clean.\n>\n> A few things to note:\n>\n> - I switched from valgrind to ASAN/LSAN early on. It's just way\n> faster, which makes the compile-test-fix cycle a lot more pleasant.\n> With these patches, you should be able to do:\n>\n> make SANITIZE=leak && (cd t && ./t1234-* -v -i)\n>\n> and get a leak report for a single script dumped to stderr.\n>\n> If you want to try it with t0000, you'll need the lock-file series I\n> just posted at:\n>\n> https://public-inbox.org/git/20170905121353.62zg3mtextmq5zrs@sigill.intra.peff.net/\n>\n> and the fix from Martin at:\n>\n> https://public-inbox.org/git/CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com/\n>\n> to get a clean run.\n>\n> - I'm using LSAN instead of the full-on ASAN because it's faster. The\n> docs warn that it's a bit experimental, and I did notice a few funny\n> behaviors (like truncated backtraces), but overall it seems fine.\n> You can also do:\n>\n> make SANITIZE=address &&\n> (cd t && ASAN_OPTIONS=abort_on_error=1 ./t1234-* -v -i)\n>\n> to do a full ASAN run (the extra environment setting is necessary to\n> override test-lib's defaults).\n>\n> - gcc's leak-checker doesn't seem to handle reachability correctly (or\n> maybe I'm holding it wrong). As a simple case, building with ASAN\n> and running git-init complains:\n>\n> $ make SANITIZE=address && ./git init foo\n> ...\n> Direct leak of 2 byte(s) in 1 object(s) allocated from:\n> #0 0x7f011dc699e0 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd99e0)\n> #1 0x558eeedbdab5 in do_xmalloc /home/peff/compile/git/wrapper.c:60\n> #2 0x558eeedbdbab in do_xmallocz /home/peff/compile/git/wrapper.c:100\n> #3 0x558eeedbdd0d in xmallocz /home/peff/compile/git/wrapper.c:108\n> #4 0x558eeedbdd0d in xmemdupz /home/peff/compile/git/wrapper.c:124\n> #5 0x558eeedbdd0d in xstrndup /home/peff/compile/git/wrapper.c:130\n> #6 0x558eeea0507a in main /home/peff/compile/git/common-main.c:39\n> #7 0x7f011cf612e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0)\n>\n> That line is the setting of argv0_path, which is a global (and thus\n> shouldn't be marked as leaking). Interestingly, it only happens with\n> -O2. Compiling with -O0 works fine. I'm not sure if it's a bug or\n> what.\n>\n> I did most of my testing with clang-6.0, which gets this case right.\n\nHmmm, I got the same wrong results (IMHO) from Valgrind, which\nclassified this as \"definitely lost\". Like you I found that -O0 helped.\nAnd yes, that was with gcc. Maybe gcc with optimization somehow manages\nto hide the pointers from these tools. I know too little about the\ntechnical details to have any real ideas, though. My searches did not\nbring up anything useful. (gcc 5.4.0)\n\nI guess clang will be my next attempt. And asan/lsan. Valgrind is slow..\n\nI'll look through this series and get back if I have any input.\n\n> - I have no idea who close or far this is to covering the whole suite.\n> Only 98 test scripts complete with no leaks. But many of those\n> failures will be hitting the same leaks over and over. It looks like\n> running \"git show' hits a few, which is going to affect a lot of\n> scripts. But bear in mind when reading the patches that this might\n> be the first step on a successful road, or it could be a dead end. :)\n>\n> Most of this is actual leak fixes. The interesting part, I think, is the\n> UNLEAK annotation in patch 10.\n>\n> [01/10]: test-lib: --valgrind should not override --verbose-log\n> [02/10]: test-lib: set LSAN_OPTIONS to abort by default\n> [03/10]: add: free leaked pathspec after add_files_to_cache()\n> [04/10]: update-index: fix cache entry leak in add_one_file()\n> [05/10]: config: plug user_config leak\n> [06/10]: reset: make tree counting less confusing\n> [07/10]: reset: free allocated tree buffers\n> [08/10]: repository: free fields before overwriting them\n> [09/10]: set_git_dir: handle feeding gitdir to itself\n> [10/10]: add UNLEAK annotation for reducing leak false positives\n>\n> Makefile | 3 +++\n> builtin/add.c | 3 +++\n> builtin/commit.c | 1 +\n> builtin/config.c | 11 +++++++++--\n> builtin/init-db.c | 2 ++\n> builtin/ls-files.c | 1 +\n> builtin/reset.c | 24 +++++++++++++++++-------\n> builtin/update-index.c | 4 +++-\n> builtin/worktree.c | 2 ++\n> environment.c | 4 +++-\n> git-compat-util.h | 7 +++++++\n> repository.c | 14 +++++++-------\n> setup.c | 5 -----\n> t/test-lib.sh | 7 ++++++-\n> usage.c | 13 +++++++++++++\n> 15 files changed, 77 insertions(+), 24 deletions(-)\n>\n> -Peff\n"},{"id":"327598","messageId":"20170905190212.4xx4xukx2bbtudij@sigill.intra.peff.net","threadId":"46704","inReplyTo":"CAN0heSrVzRPc+iVqU02qzk=DB0WT6Fscn6X-hZPFkM1TikMPVQ@mail.gmail.com","subject":"Re: [PATCH 0/10] towards clean leak-checker output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T19:02:12Z","receivedAt":"2017-09-05T19:02:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 07:50:10PM +0200, Martin Ågren wrote:\n\n> > That line is the setting of argv0_path, which is a global (and thus\n> > shouldn't be marked as leaking). Interestingly, it only happens with\n> > -O2. Compiling with -O0 works fine. I'm not sure if it's a bug or\n> > what.\n> >\n> > I did most of my testing with clang-6.0, which gets this case right.\n> \n> Hmmm, I got the same wrong results (IMHO) from Valgrind, which\n> classified this as \"definitely lost\". Like you I found that -O0 helped.\n> And yes, that was with gcc. Maybe gcc with optimization somehow manages\n> to hide the pointers from these tools. I know too little about the\n> technical details to have any real ideas, though. My searches did not\n> bring up anything useful. (gcc 5.4.0)\n\nYeah, I think it is just optimizing out the variable entirely. If\nRUNTIME_PREFIX isn't defined (and it's not for non-Windows platforms)\nthen we never look at the variable at all, and it's a dead assignment.\nAnd the compiler can see that easily because it's got static linkage. So\nit drops the variable completely, but it can't drop the call to\nxstrdup() with the information in exec_cmd.c. It has to call the\nfunction and throw away the result, resulting in the leak.\n\nIn fact, the whole extract_argv0_path thing is pointless without\nRUNTIME_PREFIX. So I think one reasonable fix is just:\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex fb94aeba9c..09f05c3bc3 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -5,7 +5,10 @@\n #define MAX_ARGS\t32\n \n static const char *argv_exec_path;\n+\n+#ifdef RUNTIME_PREFIX\n static const char *argv0_path;\n+#endif\n \n char *system_path(const char *path)\n {\n@@ -40,6 +43,7 @@ char *system_path(const char *path)\n \n void git_extract_argv0_path(const char *argv0)\n {\n+#ifdef RUNTIME_PREFIX\n \tconst char *slash;\n \n \tif (!argv0 || !*argv0)\n@@ -49,6 +53,7 @@ void git_extract_argv0_path(const char *argv0)\n \n \tif (slash)\n \t\targv0_path = xstrndup(argv0, slash - argv0);\n+#endif\n }\n \n void git_set_argv_exec_path(const char *exec_path)\n\n-Peff\n"},{"id":"327602","messageId":"CAN0heSrz=tVHj13X+yHJpAR=5o7j8EqbBdw8hvrhcnhbA04QAA@mail.gmail.com","threadId":"46704","inReplyTo":"20170905190212.4xx4xukx2bbtudij@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] towards clean leak-checker output","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-05T20:41:51Z","receivedAt":"2017-09-05T20:42:00Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 21:02, Jeff King wrote:\n> On Tue, Sep 05, 2017 at 07:50:10PM +0200, Martin Ågren wrote:\n>\n>> > That line is the setting of argv0_path, which is a global (and thus\n>> > shouldn't be marked as leaking). Interestingly, it only happens with\n>> > -O2. Compiling with -O0 works fine. I'm not sure if it's a bug or\n>> > what.\n>> >\n>> > I did most of my testing with clang-6.0, which gets this case right.\n>>\n>> Hmmm, I got the same wrong results (IMHO) from Valgrind, which\n>> classified this as \"definitely lost\". Like you I found that -O0 helped.\n>> And yes, that was with gcc. Maybe gcc with optimization somehow manages\n>> to hide the pointers from these tools. I know too little about the\n>> technical details to have any real ideas, though. My searches did not\n>> bring up anything useful. (gcc 5.4.0)\n>\n> Yeah, I think it is just optimizing out the variable entirely. If\n> RUNTIME_PREFIX isn't defined (and it's not for non-Windows platforms)\n> then we never look at the variable at all, and it's a dead assignment.\n> And the compiler can see that easily because it's got static linkage. So\n> it drops the variable completely, but it can't drop the call to\n> xstrdup() with the information in exec_cmd.c. It has to call the\n> function and throw away the result, resulting in the leak.\n\nI see. Yeah, that makes sense.\n\nFWIW, this series (combined with the other series you mentioned) makes\nt0000 and t0001 pass for me with gcc/clang. There are actually some\nleaks in t0000, they're just silently being reported to stderr, since\nthe exit statuses from git are hidden by pipes. Maybe you're already\naware of it. Depending on your definition of \"running clean\" it might be\nout of scope for this series, which is of course still very interesting\nand enlightening as it stands.\n\nOne is in cmd_show (you did mention git show) where we leak data in rev.\nThe other is some use of strdup. I can't immediately figure out how to\nget a useful stacktrace (you mentioned this as well) and it's past\nbed-time here. I'll try to play more with this tomorrow.\n\nNote for self: getting rid of all pipes would probably also help flush\nout a few leaks (or \"introduce\" them, depending on your viewpoint).\n\nMartin\n"},{"id":"327614","messageId":"CAGZ79kYmZJ8k2=SVrx9vpEXoKhRBGwu8J=fS=cYiCLdKf+nDGA@mail.gmail.com","threadId":"46704","inReplyTo":"20170905130505.him3p4jhxp64r2vy@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-05T22:05:12Z","receivedAt":"2017-09-05T22:05:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 5, 2017 at 6:05 AM, Jeff King wrote:\n\n> int main(void)\n\nnit of the day:\n s/void/int argc, char *argv/ or in case we do not\n want to emphasize the argument list s/void//\n as that adds no uninteresting things.\n\n\n>\n> In other words, you can do:\n>\n> int main(void)\n> {\n> char *p = some_function();\n> printf(\"%s\", p);\n> UNLEAK(p);\n> return 0;\n> }\n>\n> to annotate \"p\" and suppress the leak report.\n\nThis sounds really cool so far.\n\nAfter having a sneak peak at the implementation\nit is O(1) in runtime for each added element, and the\nspace complexity is O(well).\n\n> But wait, couldn't we just say \"free(p)\"? In this toy\n> example, yes. But using UNLEAK() has several advantages over\n> actually freeing the memory:\n\nThis is indeed the big question, that I have had.\n\n>\n> 1. It can be compiled conditionally. There's no need in\n> normal runs to do this free(), and it just wastes time.\n> By using a macro, we can get the benefit for leak-check\n> builds with zero cost for normal builds (this patch\n> uses a compile-time check, though we could clearly also\n> make it a run-time check at very low cost).\n>\n> Of course one could also hide free() behind a macro, so\n> this is really just arguing for having UNLEAK(), not\n> for its particular implementation.\n\nThis is only a real argument in combination with (2), or in other\nwords you seem to hint at situations like these:\n\n struct *foo = obtain_new_foo();\n ...\n #if FREE_ANNOTATED_LEAKS\n /* special free() */\n release_foo(foo);\n #endif\n\nWith UNLEAK this situation works out nicely as we just\ncopy over all memory, ignoring elements allocated inside\nfoo, but for free() we'd have issues combining the preprocessor\nmagic with the special free implementation.\n\nSo how would we use syntactic sugar to made this\nmore comfortable? Roughly like\n\n MAYBE(release_foo(foo))\n\n #if (FREE_ANNOTATED_LEAKS)\n /* we rely on strict text substitution */\n /* as the function signature may change */\n #define MAYBE(fn) fn;\n #else\n #define MAYBE(fn)\n #endif\n\nMe regurgitating this first argument is just a long way of saying\nthat it put me off even more after reading only the first argument.\nMaybe reorder this argument to show up after the current second\nargument, so the reader is guided better?\n\n> 2. It's recursive across structures. In many cases our \"p\"\n> is not just a pointer, but a complex struct whose\n> fields may have been allocated by a sub-function. And\n> in some cases (e.g., dir_struct) we don't even have a\n> function which knows how to free all of the struct\n> members.\n>\n> By marking the struct itself as reachable, that confers\n> reachability on any pointers it contains (including those\n> found in embedded structs, or reachable by walking\n> heap blocks recursively.\n>\n> 3. It works on cases where we're not sure if the value is\n> allocated or not. For example:\n>\n> char *p = argc > 1 ? argv[1] : some_function();\n>\n> It's safe to use UNLEAK(p) here, because it's not\n> freeing any memory. In the case that we're pointing to\n> argv here, the reachability checker will just ignore\n> our bytes.\n\nThis argument demonstrates why the MAYBE above is\ninferior.\n\n>\n> 4. Because it's not actually freeing memory, you can\n> UNLEAK() before we are finished accessing the variable.\n> This is helpful in cases like this:\n>\n> char *p = some_function();\n> return another_function(p);\n>\n> Writing this with free() requires:\n>\n> int ret;\n> char *p = some_function();\n> ret = another_function(p);\n> free(p);\n> return ret;\n>\n> But with unleak we can just write:\n>\n> char *p = some_function();\n> UNLEAK(p);\n> return another_function(p);\n\n 5. It's not just about worrying if we can call UNLEAK\n once (in 4), but we also do not have to worry about\n calling it twice, or recursively. (This argument can be bad\n for cargo cult programmers, but we don't have these ;-)\n\n\n\n> +#ifdef SUPPRESS_ANNOTATED_LEAKS\n> +extern void unleak_memory(const void *ptr, size_t len);\n> +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n\nAs always with macros we have to be careful about its arguments.\n\n UNLEAK(a++)\n UNLEAK(baz())\n\nwon't work as intended.\n"},{"id":"327623","messageId":"xmqqd174a9hs.fsf@gitster.mtv.corp.google.com","threadId":"46704","inReplyTo":"20170905190212.4xx4xukx2bbtudij@sigill.intra.peff.net","subject":"Re: [PATCH 0/10] towards clean leak-checker output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T01:42:07Z","receivedAt":"2017-09-06T01:42:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In fact, the whole extract_argv0_path thing is pointless without\n> RUNTIME_PREFIX. So I think one reasonable fix is just:\n>\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index fb94aeba9c..09f05c3bc3 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -5,7 +5,10 @@\n> #define MAX_ARGS\t32\n> \n> static const char *argv_exec_path;\n> +\n> +#ifdef RUNTIME_PREFIX\n> static const char *argv0_path;\n> +#endif\n> \n> char *system_path(const char *path)\n> {\n> @@ -40,6 +43,7 @@ char *system_path(const char *path)\n> \n> void git_extract_argv0_path(const char *argv0)\n> {\n> +#ifdef RUNTIME_PREFIX\n> \tconst char *slash;\n> \n> \tif (!argv0 || !*argv0)\n> @@ -49,6 +53,7 @@ void git_extract_argv0_path(const char *argv0)\n> \n> \tif (slash)\n> \t\targv0_path = xstrndup(argv0, slash - argv0);\n> +#endif\n> }\n> \n> void git_set_argv_exec_path(const char *exec_path)\n>\n> -Peff\n\nYeah, I kind of like this (I would have reduced the ifdef noise by\nleaving a totally-unused argv0_path in the BSS, though).\n\n"},{"id":"327645","messageId":"20170906122834.s7a6je4b7casdfxx@sigill.intra.peff.net","threadId":"46704","inReplyTo":"xmqqd174a9hs.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 0/2] simplifying !RUNTIME_PREFIX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T12:28:34Z","receivedAt":"2017-09-06T12:28:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 06, 2017 at 10:42:07AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > In fact, the whole extract_argv0_path thing is pointless without\n> > RUNTIME_PREFIX. So I think one reasonable fix is just:\n> [...]\n> Yeah, I kind of like this (I would have reduced the ifdef noise by\n> leaving a totally-unused argv0_path in the BSS, though).\n\nI wanted to drop it to make sure that I didn't miss any references to it\n(and that we didn't add any in the future). But -Wunused also complains\nif you leave it in. :)\n\nI think we can reorganize the code a little to make the end result more\nreadable. Like this.\n\n [1/2]: system_path: move RUNTIME_PREFIX to a sub-function\n [2/2]: git_extract_argv0_path: do nothing without RUNTIME_PREFIX\n\n exec_cmd.c | 42 ++++++++++++++++++++++++++++--------------\n 1 file changed, 28 insertions(+), 14 deletions(-)\n\n-Peff\n"},{"id":"327646","messageId":"20170906123027.w5y7s33p5p2kfzia@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170906122834.s7a6je4b7casdfxx@sigill.intra.peff.net","subject":"[PATCH 1/2] system_path: move RUNTIME_PREFIX to a sub-function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T12:30:28Z","receivedAt":"2017-09-06T12:30:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The system_path() function has an #ifdef in the middle of\nit. Let's move the conditional logic into a sub-function.\nThis isolates it more, which will make it easier to change\nand add to.\n\nSigned-off-by: Jeff King \n---\nI find the diff hard to read because it shows the opposite of what I did\n(moving the big block to its own function, rather than moving the little\nbits to a new copy of the function).\n\n exec_cmd.c | 33 +++++++++++++++++++++------------\n 1 file changed, 21 insertions(+), 12 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex fb94aeba9c..61092e9715 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -7,19 +7,12 @@\n static const char *argv_exec_path;\n static const char *argv0_path;\n \n-char *system_path(const char *path)\n-{\n #ifdef RUNTIME_PREFIX\n-\tstatic const char *prefix;\n-#else\n-\tstatic const char *prefix = PREFIX;\n-#endif\n-\tstruct strbuf d = STRBUF_INIT;\n \n-\tif (is_absolute_path(path))\n-\t\treturn xstrdup(path);\n+static const char *system_prefix(void)\n+{\n+\tstatic const char *prefix;\n \n-#ifdef RUNTIME_PREFIX\n \tassert(argv0_path);\n \tassert(is_absolute_path(argv0_path));\n \n@@ -32,9 +25,25 @@ char *system_path(const char *path)\n \t\t\t\t\"but prefix computation failed. \"\n \t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n \t}\n-#endif\n+\treturn prefix;\n+}\n+#else\n+\n+static const char *system_prefix(void)\n+{\n+\treturn PREFIX;\n+}\n+\n+#endif /* RUNTIME_PREFIX */\n+\n+char *system_path(const char *path)\n+{\n+\tstruct strbuf d = STRBUF_INIT;\n+\n+\tif (is_absolute_path(path))\n+\t\treturn xstrdup(path);\n \n-\tstrbuf_addf(&d, \"%s/%s\", prefix, path);\n+\tstrbuf_addf(&d, \"%s/%s\", system_prefix(), path);\n \treturn strbuf_detach(&d, NULL);\n }\n \n-- \n2.14.1.757.g8fad538cea\n\n"},{"id":"327647","messageId":"20170906123209.5fjexq72qz77unfr@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170906122834.s7a6je4b7casdfxx@sigill.intra.peff.net","subject":"[PATCH 2/2] git_extract_argv0_path: do nothing without RUNTIME_PREFIX","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T12:32:10Z","receivedAt":"2017-09-06T12:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the RUNTIME_PREFIX compile-time knob isn't set, we\nnever look at the argv0_path we extract. We can push its\ndeclaration inside the #ifdef to make it more clear that the\nextract code is effectively a noop.\n\nThis also un-confuses leak-checking of the argv0_path\nvariable when RUNTIME_PREFIX isn't set. The compiler is free\nto drop this static variable that we set but never look at\n(and \"gcc -O2\" does so). But the compiler still must call\nstrbuf_detach(), since it doesn't know whether that function\nhas side effects; it just throws away the result rather than\nputting it into the global.\n\nLeak-checkers which work by scanning the data segment for\npointers to heap blocks would normally consider the block\nas reachable at program end. But if the compiler removes the\nvariable entirely, there's nothing to find.\n\nSigned-off-by: Jeff King \n---\n exec_cmd.c | 33 +++++++++++++++++++--------------\n 1 file changed, 19 insertions(+), 14 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 61092e9715..ce192a2d64 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -5,9 +5,9 @@\n #define MAX_ARGS\t32\n \n static const char *argv_exec_path;\n-static const char *argv0_path;\n \n #ifdef RUNTIME_PREFIX\n+static const char *argv0_path;\n \n static const char *system_prefix(void)\n {\n@@ -27,6 +27,20 @@ static const char *system_prefix(void)\n \t}\n \treturn prefix;\n }\n+\n+void git_extract_argv0_path(const char *argv0)\n+{\n+\tconst char *slash;\n+\n+\tif (!argv0 || !*argv0)\n+\t\treturn;\n+\n+\tslash = find_last_dir_sep(argv0);\n+\n+\tif (slash)\n+\t\targv0_path = xstrndup(argv0, slash - argv0);\n+}\n+\n #else\n \n static const char *system_prefix(void)\n@@ -34,6 +48,10 @@ static const char *system_prefix(void)\n \treturn PREFIX;\n }\n \n+void git_extract_argv0_path(const char *argv0)\n+{\n+}\n+\n #endif /* RUNTIME_PREFIX */\n \n char *system_path(const char *path)\n@@ -47,19 +65,6 @@ char *system_path(const char *path)\n \treturn strbuf_detach(&d, NULL);\n }\n \n-void git_extract_argv0_path(const char *argv0)\n-{\n-\tconst char *slash;\n-\n-\tif (!argv0 || !*argv0)\n-\t\treturn;\n-\n-\tslash = find_last_dir_sep(argv0);\n-\n-\tif (slash)\n-\t\targv0_path = xstrndup(argv0, slash - argv0);\n-}\n-\n void git_set_argv_exec_path(const char *exec_path)\n {\n \targv_exec_path = exec_path;\n-- \n2.14.1.757.g8fad538cea\n"},{"id":"327648","messageId":"20170906123943.335degmzmokqrd7p@sigill.intra.peff.net","threadId":"46704","inReplyTo":"CAN0heSrz=tVHj13X+yHJpAR=5o7j8EqbBdw8hvrhcnhbA04QAA@mail.gmail.com","subject":"Re: [PATCH 0/10] towards clean leak-checker output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T12:39:43Z","receivedAt":"2017-09-06T12:39:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 10:41:51PM +0200, Martin Ågren wrote:\n\n> FWIW, this series (combined with the other series you mentioned) makes\n> t0000 and t0001 pass for me with gcc/clang. There are actually some\n> leaks in t0000, they're just silently being reported to stderr, since\n> the exit statuses from git are hidden by pipes. Maybe you're already\n> aware of it. Depending on your definition of \"running clean\" it might be\n> out of scope for this series, which is of course still very interesting\n> and enlightening as it stands.\n\nYeah, I saw those. I don't think they're worth hunting down at this\npoint. It's quite probable that the same leaks are exercised elsewhere,\nand we'll catch them later.\n\nOnce the whole test suite runs without reporting any leaks via abort(),\nI'll feel more compelled to start grepping stderr for other cases. :)\n\n> One is in cmd_show (you did mention git show) where we leak data in rev.\n> The other is some use of strdup. I can't immediately figure out how to\n> get a useful stacktrace (you mentioned this as well) and it's past\n> bed-time here. I'll try to play more with this tomorrow.\n\nI think I got funny truncated stack traces with just LSAN that went away\nwith ASAN, but I didn't dig. If you try that, remember that:\n\n - Technically just SANITIZE=address turns on the leak detector, but\n you need \"leak\" in the string to turn on the UNLEAK() magic. So use\n \"SANITIZE=address,leak\".\n\n - You'll have to set ASAN_OPTIONS=abort_on_error=1 in the environment\n manually to enable leak detection.\n\nOnce the test suite passes with leak detection on, I think we'd probably\ndo both of those automatically (but until then they make ASAN by itself\nunusable).\n\n> Note for self: getting rid of all pipes would probably also help flush\n> out a few leaks (or \"introduce\" them, depending on your viewpoint).\n\nAgreed. These leaks are really just a special form of bug. Those pipes\ncould be hiding other bugs, too. But again, I'm not too concerned. The\nplaces that pipe git are doing so because they are not meant to be\ntesting that particular command (and it should generally be covered\nelsewhere).\n\n-Peff\n"},{"id":"327650","messageId":"alpine.DEB.2.21.1.1709061522420.4132@virtualbox","threadId":"46704","inReplyTo":"20170906123027.w5y7s33p5p2kfzia@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] system_path: move RUNTIME_PREFIX to a sub-function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-06T13:23:42Z","receivedAt":"2017-09-06T13:24:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 6 Sep 2017, Jeff King wrote:\n\n> I find the diff hard to read because it shows the opposite of what I did\n> (moving the big block to its own function, rather than moving the little\n> bits to a new copy of the function).\n\nI guess --patience would have made it slightly more readable. I did not\nfind any commit in your fork on GitHub with the commit subject, else I\nwould have pasted the output of `git diff --patience` here.\n\nCiao,\nDscho\n"},{"id":"327651","messageId":"20170906132756.jj6esh6nrk7iuvjz@sigill.intra.peff.net","threadId":"46704","inReplyTo":"alpine.DEB.2.21.1.1709061522420.4132@virtualbox","subject":"Re: [PATCH 1/2] system_path: move RUNTIME_PREFIX to a sub-function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T13:27:57Z","receivedAt":"2017-09-06T13:28:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 06, 2017 at 03:23:42PM +0200, Johannes Schindelin wrote:\n\n> On Wed, 6 Sep 2017, Jeff King wrote:\n> \n> > I find the diff hard to read because it shows the opposite of what I did\n> > (moving the big block to its own function, rather than moving the little\n> > bits to a new copy of the function).\n> \n> I guess --patience would have made it slightly more readable. I did not\n> find any commit in your fork on GitHub with the commit subject, else I\n> would have pasted the output of `git diff --patience` here.\n\nI hadn't pushed it yet (but it's there now, 130217f4a). Neither\n--patience nor --histogram produces markedly different results. I think\nthe issue is just that what I actually did (moving the big inner block)\nis not nearly as minimal a diff as moving what surrounds it.\n\n-Peff\n"},{"id":"327659","messageId":"CAN0heSpYZT7cm=XNpfgcsGFa9FOR6SdaF=vXJ+M7NNaA6Mnb3g@mail.gmail.com","threadId":"46704","inReplyTo":"20170905130505.him3p4jhxp64r2vy@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-06T17:16:00Z","receivedAt":"2017-09-06T17:16:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 15:05, Jeff King wrote:\n> 1. It can be compiled conditionally. There's no need in\n> normal runs to do this free(), and it just wastes time.\n> By using a macro, we can get the benefit for leak-check\n> builds with zero cost for normal builds (this patch\n> uses a compile-time check, though we could clearly also\n> make it a run-time check at very low cost).\n>\n> Of course one could also hide free() behind a macro, so\n> this is really just arguing for having UNLEAK(), not\n> for its particular implementation.\n\nLike Stefan, I didn't quite follow 1. until after I had read the points\nbelow it. But it's still a very good commit message (as always).\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index b3b04f5dd3..de775d906c 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1819,5 +1819,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> print_summary(prefix, &oid, !current_head);\n>\n> strbuf_release(&err);\n> + UNLEAK(sb);\n> return 0;\n> }\n\nThese are both strbufs, so this ends up being a bit inconsistent. What\nwould be the ideal end state for these two and all other such\nstructures? My guess is \"always UNLEAK\", as opposed to carefully judging\nwhether foo_release() would/could add any significant overhead.\n\nIn other words, it would be ok/wanted with changes such as \"let's UNLEAK\nbar, because ..., and while at it, convert the existing foo_release to\nUNLEAK for consistency\" (or per policy, for smaller binary, whatever).\nOr \"if it ain't broken, don't fix it\"? Did you think about this, or was\nit more a random choice?\n\nMartin\n"},{"id":"327695","messageId":"20170907090018.t6ogu2vufdfhz3n6@sigill.intra.peff.net","threadId":"46704","inReplyTo":"CAN0heSpYZT7cm=XNpfgcsGFa9FOR6SdaF=vXJ+M7NNaA6Mnb3g@mail.gmail.com","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-07T09:00:18Z","receivedAt":"2017-09-07T09:00:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 06, 2017 at 07:16:00PM +0200, Martin Ågren wrote:\n\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index b3b04f5dd3..de775d906c 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -1819,5 +1819,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n> > print_summary(prefix, &oid, !current_head);\n> >\n> > strbuf_release(&err);\n> > + UNLEAK(sb);\n> > return 0;\n> > }\n> \n> These are both strbufs, so this ends up being a bit inconsistent. What\n> would be the ideal end state for these two and all other such\n> structures? My guess is \"always UNLEAK\", as opposed to carefully judging\n> whether foo_release() would/could add any significant overhead.\n> \n> In other words, it would be ok/wanted with changes such as \"let's UNLEAK\n> bar, because ..., and while at it, convert the existing foo_release to\n> UNLEAK for consistency\" (or per policy, for smaller binary, whatever).\n> Or \"if it ain't broken, don't fix it\"? Did you think about this, or was\n> it more a random choice?\n\nTo be honest, I didn't really think that deeply about it. I had a hammer\nin my hand, and LSAN kept showing me nails to pound.\n\nI agree that these two strbufs should probably be treated the same.\n\nIn general, I think I prefer using UNLEAK() because it's hard to get it\nwrong (i.e., you don't have to care about double-frees or uninitialized\npointers). For strbufs, though, that's less of an issue because they are\nalways maintained in a consistent state.\n\nAs an aside, I'm pretty sure that \"err\" can never have been allocated\nhere, and this release is always a noop. It's filled in only when we get\nan error from the ref update, which also causes us to die(). But in\ngeneral I'd prefer the code that causes readers to think the least\n(i.e., just calling free or UNLEAK here rather than forcing the reader\nto figure out whether it's possible to leak).\n\n-Peff\n"},{"id":"327697","messageId":"20170907091734.nsdpo2dpcgvf2zna@sigill.intra.peff.net","threadId":"46704","inReplyTo":"CAGZ79kYmZJ8k2=SVrx9vpEXoKhRBGwu8J=fS=cYiCLdKf+nDGA@mail.gmail.com","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-07T09:17:35Z","receivedAt":"2017-09-07T09:17:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 03:05:12PM -0700, Stefan Beller wrote:\n\n> On Tue, Sep 5, 2017 at 6:05 AM, Jeff King wrote:\n> \n> > int main(void)\n> \n> nit of the day:\n> s/void/int argc, char *argv/ or in case we do not\n> want to emphasize the argument list s/void//\n> as that adds no uninteresting things.\n\nThat really is a nit. I chose not to provide argv because it's longer\nthan \"void\" and I wasn't going to use the arguments. And I chose not to\nuse an empty argument list because it violates our style (as well as\narguably the C standard, though it leaves room for implementations to\ntake other forms of main).\n\n> > In other words, you can do:\n> >\n> > int main(void)\n> > {\n> > char *p = some_function();\n> > printf(\"%s\", p);\n> > UNLEAK(p);\n> > return 0;\n> > }\n> >\n> > to annotate \"p\" and suppress the leak report.\n> \n> This sounds really cool so far.\n> \n> After having a sneak peak at the implementation\n> it is O(1) in runtime for each added element, and the\n> space complexity is O(well).\n\nI'm not sure if your \"well\" is \"this does well\" or \"well, it could be\nquite a lot\". :)\n\nIt certainly has the potential to grow the heap without bound (since\nafter all, it's whole point is to make a giant list of variables that\nare going out of scope). But in practice we'd sprinkle this over a\nhandful of variables just before program exit (and remember that it's\ncopying only what's on the stack already; so pointers get copied, not\nwhole heap-allocated blocks).\n\nPlus it does nothing at all when not compiled with leak-checking. So I'm\nnot too worried about the extra memory usage or performance.\n\n> > 1. It can be compiled conditionally. There's no need in\n> > normal runs to do this free(), and it just wastes time.\n> > By using a macro, we can get the benefit for leak-check\n> > builds with zero cost for normal builds (this patch\n> > uses a compile-time check, though we could clearly also\n> > make it a run-time check at very low cost).\n> >\n> > Of course one could also hide free() behind a macro, so\n> > this is really just arguing for having UNLEAK(), not\n> > for its particular implementation.\n> \n> This is only a real argument in combination with (2), or in other\n> words you seem to hint at situations like these:\n\nWell, the numbered list was meant to be a set of arguments, each of\nwhich contributes to the overall conclusion. :) I agree that (1) is the\nweakest. Since both you and Martin seemed to get hung up on it, I'll\nre-organize it a bit for the re-roll.\n\n> 5. It's not just about worrying if we can call UNLEAK\n> once (in 4), but we also do not have to worry about\n> calling it twice, or recursively. (This argument can be bad\n> for cargo cult programmers, but we don't have these ;-)\n\nTrue. I didn't come across that case in any of the ones I converted. As\na more general rule, UNLEAK() doesn't access any pointed-to memory at\nall. So it's fine with already-freed or even uninitialized memory (which\nof course is technically wrong according to the standard, but in\npractice would be fine, as we'd copy garbage that does not match a heap\nblock).\n\n> > +#ifdef SUPPRESS_ANNOTATED_LEAKS\n> > +extern void unleak_memory(const void *ptr, size_t len);\n> > +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n> \n> As always with macros we have to be careful about its arguments.\n> \n> UNLEAK(a++)\n> UNLEAK(baz())\n> \n> won't work as intended.\n\nYes, I intended this to be used only for actual variables. I couldn't\nthink of a way to enforce that at compile time with some kind of\nBUILD_ASSERT (even requiring an lvalue isn't quite strict enough).\n\n-Peff\n"},{"id":"327725","messageId":"20170907190608.GA59643@google.com","threadId":"46704","inReplyTo":"20170905130501.xwc2ud2pwpbhqsli@sigill.intra.peff.net","subject":"Re: [PATCH 09/10] set_git_dir: handle feeding gitdir to itself","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-09-07T19:06:08Z","receivedAt":"2017-09-07T19:06:19Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 09/05, Jeff King wrote:\n> Ideally we'd free the existing gitdir field before assigning\n> the new one, to avoid a memory leak. But we can't do so\n> safely because some callers do the equivalent of:\n> \n> set_git_dir(get_git_dir());\n> \n> We can detect that case as a noop, but there are even more\n> complicated cases like:\n> \n> set_git_dir(remove_leading_path(worktree, get_git_dir());\n> \n> where we really do need to do some work, but the original\n> string must remain valid.\n> \n> Rather than put the burden on callers to make a copy of the\n> string (only to free it later, since we'll make a copy of it\n> ourselves), let's solve the problem inside set_git_dir(). We\n> can make a copy of the pointer for the old gitdir, and then\n> avoid freeing it until after we've made our new copy.\n> \n\nThis patch and the one before it look good to me. Thanks for fixing\nthis issue!\n\n> Signed-off-by: Jeff King \n> ---\n> repository.c | 10 +++-------\n> setup.c | 5 -----\n> 2 files changed, 3 insertions(+), 12 deletions(-)\n> \n> diff --git a/repository.c b/repository.c\n> index 52f1821c6b..97c732bd48 100644\n> --- a/repository.c\n> +++ b/repository.c\n> @@ -56,16 +56,12 @@ static void repo_setup_env(struct repository *repo)\n> void repo_set_gitdir(struct repository *repo, const char *path)\n> {\n> \tconst char *gitfile = read_gitfile(path);\n> +\tchar *old_gitdir = repo->gitdir;\n> \n> -\t/*\n> -\t * NEEDSWORK: Eventually we want to be able to free gitdir and the rest\n> -\t * of the environment before reinitializing it again, but we have some\n> -\t * crazy code paths where we try to set gitdir with the current gitdir\n> -\t * and we don't want to free gitdir before copying the passed in value.\n> -\t */\n> \trepo->gitdir = xstrdup(gitfile ? gitfile : path);\n> -\n> \trepo_setup_env(repo);\n> +\n> +\tfree(old_gitdir);\n> }\n> \n> /*\n> diff --git a/setup.c b/setup.c\n> index 23950173fc..6d8380acd2 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -399,11 +399,6 @@ void setup_work_tree(void)\n> \tif (getenv(GIT_WORK_TREE_ENVIRONMENT))\n> \t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, \".\", 1);\n> \n> -\t/*\n> -\t * NEEDSWORK: this call can essentially be set_git_dir(get_git_dir())\n> -\t * which can cause some problems when trying to free the old value of\n> -\t * gitdir.\n> -\t */\n> \tset_git_dir(remove_leading_path(git_dir, work_tree));\n> \tinitialized = 1;\n> }\n> -- \n> 2.14.1.721.gc5bc1565f1\n> \n\n-- \nBrandon Williams\n"},{"id":"327732","messageId":"CAGZ79kahVcKY3mJdZAP3jWCBY58VbCTHCfaonTgP7TDYrV+jag@mail.gmail.com","threadId":"46704","inReplyTo":"20170907091734.nsdpo2dpcgvf2zna@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-07T20:38:51Z","receivedAt":"2017-09-07T20:39:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Sep 7, 2017 at 2:17 AM, Jeff King wrote:\n>> After having a sneak peak at the implementation\n>> it is O(1) in runtime for each added element, and the\n>> space complexity is O(well).\n>\n> I'm not sure if your \"well\" is \"this does well\" or \"well, it could be\n> quite a lot\". :)\n\nBoth actually. When I wrote it I thought the phonetic interpretation\nwas way too funny, but nobody can hear subtle humor on mailing\nlists. :)\n\nIf UNLEAK is used correctly, then it sounds more like\n\"this does well (and we cannot do better anyway)\".\n\n> It certainly has the potential to grow the heap without bound (since\n> after all, it's whole point is to make a giant list of variables that\n> are going out of scope). But in practice we'd sprinkle this over a\n> handful of variables just before program exit (and remember that it's\n> copying only what's on the stack already; so pointers get copied, not\n> whole heap-allocated blocks).\n>\n> Plus it does nothing at all when not compiled with leak-checking. So I'm\n> not too worried about the extra memory usage or performance.\n\nme neither.\n\nThanks for starting this series (I am really happy about this solution)!\nStefan\n"},{"id":"327752","messageId":"20170908063841.wb23ibs2ancdct2v@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net","subject":"[PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-08T06:38:41Z","receivedAt":"2017-09-08T06:38:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 09:01:50AM -0400, Jeff King wrote:\n\n> Most of this is actual leak fixes. The interesting part, I think, is the\n> UNLEAK annotation in patch 10.\n> \n> [01/10]: test-lib: --valgrind should not override --verbose-log\n> [02/10]: test-lib: set LSAN_OPTIONS to abort by default\n> [03/10]: add: free leaked pathspec after add_files_to_cache()\n> [04/10]: update-index: fix cache entry leak in add_one_file()\n> [05/10]: config: plug user_config leak\n> [06/10]: reset: make tree counting less confusing\n> [07/10]: reset: free allocated tree buffers\n> [08/10]: repository: free fields before overwriting them\n> [09/10]: set_git_dir: handle feeding gitdir to itself\n> [10/10]: add UNLEAK annotation for reducing leak false positives\n\nHere's a re-roll of just patch 10, which is the only one that got any\ncritical feedback. This version:\n\n 1. Only compiles the definition of unleak_memory() when it will be\n used. Thanks, Ramsay.\n\n 2. Tweaks the \"why UNLEAK() is better than free()\" to put the\n compelling arguments first. Thanks Stefan and Martin.\n\n 3. I added an in-code comment to explain how UNLEAK() should be used.\n I resisted this at first because the commit message goes into more\n detail on how it works and the implications. But it's probably good\n to have at least some guidance for people who stumble on it in the\n code.\n\n 4. Tweaks the examples to avoid the \"argc vs void\" question. :) I\n also made them \"cmd_foo()\" which is a more realistic example of\n when you'd want to use this, since our commands typically do not\n implement their own main() anyway.\n\n-- >8 --\nSubject: add UNLEAK annotation for reducing leak false positives\n\nIt's a common pattern in git commands to allocate some\nmemory that should last for the lifetime of the program and\nthen not bother to free it, relying on the OS to throw it\naway.\n\nThis keeps the code simple, and it's fast (we don't waste\ntime traversing structures or calling free at the end of the\nprogram). But it also triggers warnings from memory-leak\ncheckers like valgrind or LSAN. They know that the memory\nwas still allocated at program exit, but they don't know\n_when_ the leaked memory stopped being useful. If it was\nearly in the program, then it's probably a real and\nimportant leak. But if it was used right up until program\nexit, it's not an interesting leak and we'd like to suppress\nit so that we can see the real leaks.\n\nThis patch introduces an UNLEAK() macro that lets us do so.\nTo understand its design, let's first look at some of the\nalternatives.\n\nUnfortunately the suppression systems offered by\nleak-checking tools don't quite do what we want. A\nleak-checker basically knows two things:\n\n 1. Which blocks were allocated via malloc, and the\n callstack during the allocation.\n\n 2. Which blocks were left un-freed at the end of the\n program (and which are unreachable, but more on that\n later).\n\nTheir suppressions work by mentioning the function or\ncallstack of a particular allocation, and marking it as OK\nto leak. So imagine you have code like this:\n\n int cmd_foo(...)\n {\n\t/* this allocates some memory */\n\tchar *p = some_function();\n\tprintf(\"%s\", p);\n\treturn 0;\n }\n\nYou can say \"ignore allocations from some_function(),\nthey're not leaks\". But that's not right. That function may\nbe called elsewhere, too, and we would potentially want to\nknow about those leaks.\n\nSo you can say \"ignore the callstack when main calls\nsome_function\". That works, but your annotations are\nbrittle. In this case it's only two functions, but you can\nimagine that the actual allocation is much deeper. If any of\nthe intermediate code changes, you have to update the\nsuppression.\n\nWhat we _really_ want to say is that \"the value assigned to\np at the end of the function is not a real leak\". But\nleak-checkers can't understand that; they don't know about\n\"p\" in the first place.\n\nHowever, we can do something a little bit tricky if we make\nsome assumptions about how leak-checkers work. They\ngenerally don't just report all un-freed blocks. That would\nreport even globals which are still accessible when the\nleak-check is run. Instead they take some set of memory\n(like BSS) as a root and mark it as \"reachable\". Then they\nscan the reachable blocks for anything that looks like a\npointer to a malloc'd block, and consider that block\nreachable. And then they scan those blocks, and so on,\ntransitively marking anything reachable from a global as\n\"not leaked\" (or at least leaked in a different category).\n\nSo we can mark the value of \"p\" as reachable by putting it\ninto a variable with program lifetime. One way to do that is\nto just mark \"p\" as static. But that actually affects the\nrun-time behavior if the function is called twice (you\naren't likely to call main() twice, but some of our cmd_*()\nfunctions are called from other commands).\n\nInstead, we can trick the leak-checker by putting the value\ninto _any_ reachable bytes. This patch keeps a global\nlinked-list of bytes copied from \"unleaked\" variables. That\nlist is reachable even at program exit, which confers\nrecursive reachability on whatever values we unleak.\n\nIn other words, you can do:\n\n int cmd_foo(...)\n {\n\tchar *p = some_function();\n\tprintf(\"%s\", p);\n\tUNLEAK(p);\n\treturn 0;\n }\n\nto annotate \"p\" and suppress the leak report.\n\nBut wait, couldn't we just say \"free(p)\"? In this toy\nexample, yes. But UNLEAK()'s byte-copying strategy has\nseveral advantages over actually freeing the memory:\n\n 1. It's recursive across structures. In many cases our \"p\"\n is not just a pointer, but a complex struct whose\n fields may have been allocated by a sub-function. And\n in some cases (e.g., dir_struct) we don't even have a\n function which knows how to free all of the struct\n members.\n\n By marking the struct itself as reachable, that confers\n reachability on any pointers it contains (including those\n found in embedded structs, or reachable by walking\n heap blocks recursively.\n\n 2. It works on cases where we're not sure if the value is\n allocated or not. For example:\n\n char *p = argc > 1 ? argv[1] : some_function();\n\n It's safe to use UNLEAK(p) here, because it's not\n freeing any memory. In the case that we're pointing to\n argv here, the reachability checker will just ignore\n our bytes.\n\n 3. Likewise, it works even if the variable has _already_\n been freed. We're just copying the pointer bytes. If\n the block has been freed, the leak-checker will skip\n over those bytes as uninteresting.\n\n 4. Because it's not actually freeing memory, you can\n UNLEAK() before we are finished accessing the variable.\n This is helpful in cases like this:\n\n char *p = some_function();\n return another_function(p);\n\n Writing this with free() requires:\n\n int ret;\n char *p = some_function();\n ret = another_function(p);\n free(p);\n return ret;\n\n But with unleak we can just write:\n\n char *p = some_function();\n UNLEAK(p);\n return another_function(p);\n\nThis patch adds the UNLEAK() macro and enables it\nautomatically when Git is compiled with SANITIZE=leak. In\nnormal builds it's a noop, so we pay no runtime cost.\n\nIt also adds some UNLEAK() annotations to show off how the\nfeature works. On top of other recent leak fixes, these are\nenough to get t0000 and t0001 to pass when compiled with\nLSAN.\n\nNote the case in commit.c which actually converts a\nstrbuf_release() into an UNLEAK. This code was already\nnon-leaky, but the free didn't do anything useful, since\nwe're exiting. Converting it to an annotation means that\nnon-leak-checking builds pay no runtime cost. The cost is\nminimal enough that it's probably not worth going on a\ncrusade to convert these kinds of frees to UNLEAKS. I did it\nhere for consistency with the \"sb\" leak (though it would\nhave been equally correct to go the other way, and turn them\nboth into strbuf_release() calls).\n\nSigned-off-by: Jeff King \n---\n Makefile | 3 +++\n builtin/add.c | 2 ++\n builtin/commit.c | 3 ++-\n builtin/config.c | 4 ++++\n builtin/init-db.c | 2 ++\n builtin/ls-files.c | 1 +\n builtin/worktree.c | 2 ++\n git-compat-util.h | 20 ++++++++++++++++++++\n usage.c | 15 +++++++++++++++\n 9 files changed, 51 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex f2bb7f2f63..c052f09bba 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1036,6 +1036,9 @@ BASIC_CFLAGS += -fno-omit-frame-pointer\n ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n endif\n+ifneq ($(filter leak,$(SANITIZERS)),)\n+BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n+endif\n endif\n \n ifndef sysconfdir\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ef625e3fb8..a648cf4c56 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -515,5 +515,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Unable to write new index file\"));\n \t}\n \n+\tUNLEAK(pathspec);\n+\tUNLEAK(dir);\n \treturn exit_status;\n }\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b3b04f5dd3..58f9747c2f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1818,6 +1818,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tif (!quiet)\n \t\tprint_summary(prefix, &oid, !current_head);\n \n-\tstrbuf_release(&err);\n+\tUNLEAK(err);\n+\tUNLEAK(sb);\n \treturn 0;\n }\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 52a4606243..d13daeeb55 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -631,6 +631,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(_(\"cannot overwrite multiple values with a single value\\n\"\n@@ -641,6 +642,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2], 0);\n \t}\n@@ -648,6 +650,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value,\n \t\t\t\t\t\t\t CONFIG_REGEX_NONE, 0);\n@@ -656,6 +659,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 3);\n \t\tvalue = normalize_value(argv[0], argv[1]);\n+\t\tUNLEAK(value);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2], 1);\n \t}\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 47823f9aa4..c9b7946bad 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -579,6 +579,8 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)\n \t\t\tset_git_work_tree(work_tree);\n \t}\n \n+\tUNLEAK(real_git_dir);\n+\n \tflags |= INIT_DB_EXIST_OK;\n \treturn init_db(git_dir, real_git_dir, template_dir, flags);\n }\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex e1339e6d17..8c713c47ac 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -673,5 +673,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\treturn bad ? 1 : 0;\n \t}\n \n+\tUNLEAK(dir);\n \treturn 0;\n }\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex c98e2ce5f5..de26849f55 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -381,6 +381,8 @@ static int add(int ac, const char **av, const char *prefix)\n \t\tbranch = opts.new_branch;\n \t}\n \n+\tUNLEAK(path);\n+\tUNLEAK(opts);\n \treturn add_worktree(path, branch, &opts);\n }\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 6678b488cc..003e444c46 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1169,4 +1169,24 @@ static inline int is_missing_file_error(int errno_)\n \n extern int cmd_main(int, const char **);\n \n+/*\n+ * You can mark a stack variable with UNLEAK(var) to avoid it being\n+ * reported as a leak by tools like LSAN or valgrind. The argument\n+ * should generally be the variable itself (not its address and not what\n+ * it points to). It's safe to use this on pointers which may already\n+ * have been freed, or on pointers which may still be in use.\n+ *\n+ * Use this _only_ for a variable that leaks by going out of scope at\n+ * program exit (so only from cmd_* functions or their direct helpers).\n+ * Normal functions, especially those which may be called multiple\n+ * times, should actually free their memory. This is only meant as\n+ * an annotation, and does nothing in non-leak-checking builds.\n+ */\n+#ifdef SUPPRESS_ANNOTATED_LEAKS\n+extern void unleak_memory(const void *ptr, size_t len);\n+#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n+#else\n+#define UNLEAK(var)\n+#endif\n+\n #endif\ndiff --git a/usage.c b/usage.c\nindex 1ea7df9a20..cdd534c9df 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -241,3 +241,18 @@ NORETURN void BUG(const char *fmt, ...)\n \tva_end(ap);\n }\n #endif\n+\n+#ifdef SUPPRESS_ANNOTATED_LEAKS\n+void unleak_memory(const void *ptr, size_t len)\n+{\n+\tstatic struct suppressed_leak_root {\n+\t\tstruct suppressed_leak_root *next;\n+\t\tchar data[FLEX_ARRAY];\n+\t} *suppressed_leaks;\n+\tstruct suppressed_leak_root *root;\n+\n+\tFLEX_ALLOC_MEM(root, data, ptr, len);\n+\troot->next = suppressed_leaks;\n+\tsuppressed_leaks = root;\n+}\n+#endif\n-- \n2.14.1.769.g4f4ea7dfd3\n\n"},{"id":"327890","messageId":"1505223698.27800.30.camel@gmail.com","threadId":"46704","inReplyTo":"20170905130505.him3p4jhxp64r2vy@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-12T13:41:38Z","receivedAt":"2017-09-12T13:41:40Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tue, 2017-09-05 at 09:05 -0400, Jeff King wrote:\n> This patch introduces an UNLEAK() macro that lets us do so.\n> To understand its design, let's first look at some of the\n> alternatives.\n> \n\n> This patch adds the UNLEAK() macro and enables it\n> automatically when Git is compiled with SANITIZE=leak.\n> It adds some UNLEAK() annotations to show off how the\n> feature works. On top of other recent leak fixes, these are\n> enough to get t0000 and t0001 to pass when compiled with\n> LSAN.\n\nMy nit of the day ;-)\n\nThe above paragraphs seems to going against the following guideline of\nDocumentation/SubmittingPatches,\n\n Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n instead of \"[This patch] makes xyzzy do frotz\" or \"[I] changed xyzzy\n to do frotz\", as if you are giving orders to the codebase to change\n its behavior. Try to make sure your explanation can be understood\n\n\n\n -- \n Kaartic\n"},{"id":"327891","messageId":"1505226892.27800.37.camel@gmail.com","threadId":"46704","inReplyTo":"CAGZ79kYmZJ8k2=SVrx9vpEXoKhRBGwu8J=fS=cYiCLdKf+nDGA@mail.gmail.com","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-12T14:34:52Z","receivedAt":"2017-09-12T14:34:54Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"> On Tue, 2017-09-05 at 15:05 -0700, Stefan Beller wrote:\n> \n> After having a sneak peak at the implementation\n> it is O(1) in runtime for each added element, and the\n> space complexity is O(well).\n> \n\nIncidentally I was reading about \"complexity of algorithms\" and there\nwas the following paragraph in the book,\n\n\n Unfortunately, as Knuth observed, big-O notation is often used by careless writers and\n speakers as if it had the same meaning as big-Theta notation. Keep this in mind when you see\n big-O notation used. The recent trend has been to use big-Theta notation whenever both upper\n and lower bounds on the size of a function are needed.\n\n\nSo, if my interpretation is correct the above usage of O(1) and O(well)\nshould have been Θ(1) and Θ(well).\n\n-- \nKaartic\n"},{"id":"327892","messageId":"20170912150541.fkgnxddv6z223h3d@sigill.intra.peff.net","threadId":"46704","inReplyTo":"1505226892.27800.37.camel@gmail.com","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-12T15:05:42Z","receivedAt":"2017-09-12T15:05:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 12, 2017 at 08:04:52PM +0530, Kaartic Sivaraam wrote:\n\n> > On Tue, 2017-09-05 at 15:05 -0700, Stefan Beller wrote:\n> > \n> > After having a sneak peak at the implementation\n> > it is O(1) in runtime for each added element, and the\n> > space complexity is O(well).\n> > \n> \n> Incidentally I was reading about \"complexity of algorithms\" and there\n> was the following paragraph in the book,\n> \n> \n> Unfortunately, as Knuth observed, big-O notation is often used by careless writers and\n> speakers as if it had the same meaning as big-Theta notation. Keep this in mind when you see\n> big-O notation used. The recent trend has been to use big-Theta notation whenever both upper\n> and lower bounds on the size of a function are needed.\n> \n> So, if my interpretation is correct the above usage of O(1) and O(well)\n> should have been Θ(1) and Θ(well).\n\nBut theta-well isn't a pun. :P\n\nIt is true that prepending to a linked list is also Θ(1), but I'm not\nsure if it's carelessness that causes many programmers to use big-O.\nIt's that what we care about is worst-case performance. So knowing that\nwe have a lower bound isn't usually that interesting. What we want to\nknow is whether it will blow up in our face as \"n\" gets large.\n\nPlus typing non-ascii characters is annoying. :)\n\nIf you want to talk about sloppy analysis, the two most common problems\nI see are:\n\n 1. People talk about big-O complexity without discussing constants.\n For reasonable values of \"n\", the constants often matter (they're\n not wrong about big-O, but they are wrong about what will run fast\n in practice).\n\n 2. Glossing over things like amortized costs. Hash tables aren't\n really O(1) because they eventually fill up and get collisions. You\n have to talk about load factor, resizing, etc.\n\nI'm sure I'm guilty of doing those things sometimes, too.\n\n-Peff\n"},{"id":"327893","messageId":"20170912152935.iqmj6jpkvgifaju2@sigill.intra.peff.net","threadId":"46704","inReplyTo":"1505223698.27800.30.camel@gmail.com","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-12T15:29:35Z","receivedAt":"2017-09-12T15:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 12, 2017 at 07:11:38PM +0530, Kaartic Sivaraam wrote:\n\n> On Tue, 2017-09-05 at 09:05 -0400, Jeff King wrote:\n> > This patch introduces an UNLEAK() macro that lets us do so.\n> > To understand its design, let's first look at some of the\n> > alternatives.\n> > \n> \n> > This patch adds the UNLEAK() macro and enables it\n> > automatically when Git is compiled with SANITIZE=leak.\n> > It adds some UNLEAK() annotations to show off how the\n> > feature works. On top of other recent leak fixes, these are\n> > enough to get t0000 and t0001 to pass when compiled with\n> > LSAN.\n> \n> My nit of the day ;-)\n> \n> The above paragraphs seems to going against the following guideline of\n> Documentation/SubmittingPatches,\n> \n> Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n> instead of \"[This patch] makes xyzzy do frotz\" or \"[I] changed xyzzy\n> to do frotz\", as if you are giving orders to the codebase to change\n> its behavior. Try to make sure your explanation can be understood\n\nLike all good writing rules, I think it's important to know when to\nbreak them. :)\n\nWriting in the imperative is _most_ important in the subject. You're\nlikely to see a lot of subjects in a list, and it makes the list easier\nto read if they all match. It also tends to be shorter, which is good\nfor subjects.\n\nFor short commit messages, I think the imperative also keeps things\ntight and to the point: describe the problem and then say how to fix it.\nThe recent 0db3dc75f is a good example (which I picked by skimming\nrecent \"git log\" output). But saying \"this patch\" is IMHO not that big a\nproblem there, as long as it isn't done excessively.\n\nWhen you the explanation is longer or more complicated, the imperative\ncan actually be a bit _too_ terse. In longer text it helps to guide\nreaders in the direction you want their thoughts to take. Having a\nthree-paragraph explanation of the problem or current state of things\nand then jumping right into \"Do this. Do that.\" lacks context. A marker\nlike \"this patch\" helps the reader know that you're switching gears to\ntalking about the solution.\n\nI also think that \"I\" is useful in avoiding the passive voice. It can\ncertainly be used gratuitously and make things less clear, but in most\ncases I'd rather see something like \"I tested performance under these\nconditions\" than \"Performance was tested under these conditions\". I also\noften use the \"academic we\" here even when I worked on something myself.\n\n-Peff\n"},{"id":"327913","messageId":"26c0badc-0443-fca0-a12b-033a87dcffb1@gmail.com","threadId":"46704","inReplyTo":"20170912152935.iqmj6jpkvgifaju2@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-13T06:44:40Z","receivedAt":"2017-09-13T06:44:41Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tuesday 12 September 2017 08:59 PM, Jeff King wrote:\n> Like all good writing rules, I think it's important to know when to\n> break them. :)\n\nThat's right. \"Have guidelines but 'Be bold' enough to break them when\nthey seem to be inducing counter productivity.\"\n\n> Writing in the imperative is _most_ important in the subject. You're\n> likely to see a lot of subjects in a list, and it makes the list easier\n> to read if they all match. It also tends to be shorter, which is good\n> for subjects.\n>\n> For short commit messages, I think the imperative also keeps things\n> tight and to the point: describe the problem and then say how to fix it.\n> The recent 0db3dc75f is a good example (which I picked by skimming\n> recent \"git log\" output). But saying \"this patch\" is IMHO not that big a\n> problem there, as long as it isn't done excessively.\n>\n> When you the explanation is longer or more complicated, the imperative\n> can actually be a bit _too_ terse. In longer text it helps to guide\n> readers in the direction you want their thoughts to take. Having a\n> three-paragraph explanation of the problem or current state of things\n> and then jumping right into \"Do this. Do that.\" lacks context. A marker\n> like \"this patch\" helps the reader know that you're switching gears to\n> talking about the solution.\n>\n> I also think that \"I\" is useful in avoiding the passive voice. It can\n> certainly be used gratuitously and make things less clear, but in most\n> cases I'd rather see something like \"I tested performance under these\n> conditions\" than \"Performance was tested under these conditions\". I also\n> often use the \"academic we\" here even when I worked on something myself.\n\nThanks for taking the time to give the detailed and clear explanation.\n\n---\nKaartic\n"},{"id":"327914","messageId":"2320e1ea-4a8a-543c-7b9e-c40d6c07288a@gmail.com","threadId":"46704","inReplyTo":"20170912150541.fkgnxddv6z223h3d@sigill.intra.peff.net","subject":"Re: [PATCH 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Kaartic Sivaraam","fromEmail":"kaarticsivaraam91196@gmail.com","sentAt":"2017-09-13T07:13:14Z","receivedAt":"2017-09-13T07:13:18Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Tuesday 12 September 2017 08:35 PM, Jeff King wrote:\n> But theta-well isn't a pun. :P \n\n:)\n\n> It is true that prepending to a linked list is also Θ(1), but I'm not\n> sure if it's carelessness that causes many programmers to use big-O.\n> It's that what we care about is worst-case performance. So knowing that\n> we have a lower bound isn't usually that interesting. What we want to\n> know is whether it will blow up in our face as \"n\" gets large.\n\nThis seems quite acceptable.\n\n> Plus typing non-ascii characters is annoying. :)\n\nI expected this to be a reason. :)\n\n---\nKaartic\n"},{"id":"328425","messageId":"20170919134552.7845f021@twelve2.svl.corp.google.com","threadId":"46704","inReplyTo":"20170908063841.wb23ibs2ancdct2v@sigill.intra.peff.net","subject":"Re: [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-09-19T20:45:52Z","receivedAt":"2017-09-19T20:45:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks - this does look like a good thing to have. Sorry for the late\ncomments.\n\nThe following comments are assuming that we're going to standardize on\nUNLEAK(var); (with the semicolon).\n\nOn Fri, 8 Sep 2017 02:38:41 -0400\nJeff King wrote:\n\n> +#ifdef SUPPRESS_ANNOTATED_LEAKS\n> +extern void unleak_memory(const void *ptr, size_t len);\n> +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n\nI would feel better if the semicolon was omitted. I don't think it\nmatters in this particular case, though.\n\n> +#else\n> +#define UNLEAK(var)\n\nI think this should be defined to be something (for example, \"do {}\nwhile (0)\"), at least so that we have compiler errors when UNLEAK(var)\nis used incorrectly (for example, without the semicolon) when\nSUPPRESS_ANNOTATED_LEAKS is not defined.\n"},{"id":"328426","messageId":"20170919210300.cbrdjqor6xuwd7bs@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170919134552.7845f021@twelve2.svl.corp.google.com","subject":"Re: [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T21:03:01Z","receivedAt":"2017-09-19T21:03:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 19, 2017 at 01:45:52PM -0700, Jonathan Tan wrote:\n\n> The following comments are assuming that we're going to standardize on\n> UNLEAK(var); (with the semicolon).\n\nYeah, I assumed we would. We don't have to, since this really is sort-of\nmagical, but I think the code looks better with it.\n\n> On Fri, 8 Sep 2017 02:38:41 -0400\n> Jeff King wrote:\n> \n> > +#ifdef SUPPRESS_ANNOTATED_LEAKS\n> > +extern void unleak_memory(const void *ptr, size_t len);\n> > +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n> \n> I would feel better if the semicolon was omitted. I don't think it\n> matters in this particular case, though.\n\nYou end up with a double semi-colon. Some linters might complain.\n\n> > +#else\n> > +#define UNLEAK(var)\n> \n> I think this should be defined to be something (for example, \"do {}\n> while (0)\"), at least so that we have compiler errors when UNLEAK(var)\n> is used incorrectly (for example, without the semicolon) when\n> SUPPRESS_ANNOTATED_LEAKS is not defined.\n\nSeems reasonable.\n\nI think both are worth doing, but the patch is already in next so we\nneed to do it on top. Do you want to prepare a patch?\n\n-Peff\n"},{"id":"328428","messageId":"20170919213456.13983-1-jonathantanmy@google.com","threadId":"46704","inReplyTo":"20170919210300.cbrdjqor6xuwd7bs@sigill.intra.peff.net","subject":"[PATCH for jk/leak-checkers] git-compat-util: make UNLEAK less error-prone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-09-19T21:34:56Z","receivedAt":"2017-09-19T21:35:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 0e5bba5 (\"add UNLEAK annotation for reducing leak false\npositives\", 2017-09-08) introduced an UNLEAK macro to be used as\n\"UNLEAK(var);\", but its existing definitions make it possible to be\ninvoked as \"UNLEAK(var)\" (without the trailing semicolon) too.\n\nTherefore, modify its definitions to cause a compile-time error if\ninvoked without the trailing semicolon.\n\nSigned-off-by: Jonathan Tan \n---\nSure, here is the patch.\n---\n git-compat-util.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 003e444c4..9bc15b036 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1184,9 +1184,9 @@ extern int cmd_main(int, const char **);\n */\n #ifdef SUPPRESS_ANNOTATED_LEAKS\n extern void unleak_memory(const void *ptr, size_t len);\n-#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n+#define UNLEAK(var) unleak_memory(&(var), sizeof(var))\n #else\n-#define UNLEAK(var)\n+#define UNLEAK(var) do {} while (0)\n #endif\n \n #endif\n-- \n2.14.1.821.g8fa685d3b7-goog\n\n"},{"id":"328429","messageId":"20170919214609.beair2zraemibai2@sigill.intra.peff.net","threadId":"46704","inReplyTo":"20170919213456.13983-1-jonathantanmy@google.com","subject":"Re: [PATCH for jk/leak-checkers] git-compat-util: make UNLEAK less error-prone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T21:46:09Z","receivedAt":"2017-09-19T21:46:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 19, 2017 at 02:34:56PM -0700, Jonathan Tan wrote:\n\n> Commit 0e5bba5 (\"add UNLEAK annotation for reducing leak false\n> positives\", 2017-09-08) introduced an UNLEAK macro to be used as\n> \"UNLEAK(var);\", but its existing definitions make it possible to be\n> invoked as \"UNLEAK(var)\" (without the trailing semicolon) too.\n> \n> Therefore, modify its definitions to cause a compile-time error if\n> invoked without the trailing semicolon.\n\nThe patch itself looks good, but I think your commit message dances\naround the real motivation: some parsers may complain about doubled\nsemicolons, or semicolons without an associated line of code (which is\nreally just a doubled semicolon, too, I guess).\n\n-Peff\n"},{"id":"328430","messageId":"20170919221004.19047-1-jonathantanmy@google.com","threadId":"46704","inReplyTo":"20170919214609.beair2zraemibai2@sigill.intra.peff.net","subject":"[PATCH for jk/leak-checkers v2] git-compat-util: make UNLEAK less error-prone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-09-19T22:10:04Z","receivedAt":"2017-09-19T22:10:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 0e5bba5 (\"add UNLEAK annotation for reducing leak false\npositives\", 2017-09-08) introduced an UNLEAK macro to be used as\n\"UNLEAK(var);\", but its existing definitions leave semicolons that act\nas empty statements, which may cause some tools to complain.\n\nTherefore, modify its definitions to not leave these semicolons.\n\nSigned-off-by: Jonathan Tan \n---\nOK, here is the same patch with an updated commit message.\n---\n git-compat-util.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 003e444c4..9bc15b036 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1184,9 +1184,9 @@ extern int cmd_main(int, const char **);\n */\n #ifdef SUPPRESS_ANNOTATED_LEAKS\n extern void unleak_memory(const void *ptr, size_t len);\n-#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n+#define UNLEAK(var) unleak_memory(&(var), sizeof(var))\n #else\n-#define UNLEAK(var)\n+#define UNLEAK(var) do {} while (0)\n #endif\n \n #endif\n-- \n2.14.1.821.g8fa685d3b7-goog\n\n"},{"id":"328436","messageId":"xmqqzi9qp2hq.fsf@gitster.mtv.corp.google.com","threadId":"46704","inReplyTo":"20170919210300.cbrdjqor6xuwd7bs@sigill.intra.peff.net","subject":"Re: [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T01:45:05Z","receivedAt":"2017-09-20T01:45:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 19, 2017 at 01:45:52PM -0700, Jonathan Tan wrote:\n>\n>> The following comments are assuming that we're going to standardize on\n>> UNLEAK(var); (with the semicolon).\n>\n> Yeah, I assumed we would. We don't have to, since this really is sort-of\n> magical, but I think the code looks better with it.\n>\n>> On Fri, 8 Sep 2017 02:38:41 -0400\n>> Jeff King wrote:\n>> \n>> > +#ifdef SUPPRESS_ANNOTATED_LEAKS\n>> > +extern void unleak_memory(const void *ptr, size_t len);\n>> > +#define UNLEAK(var) unleak_memory(&(var), sizeof(var));\n>> \n>> I would feel better if the semicolon was omitted. I don't think it\n>> matters in this particular case, though.\n>\n> You end up with a double semi-colon. Some linters might complain.\n\nYeah, and it makes\n\n\tif (some condition)\n\t\tUNLEAK(var);\n\telse\n\t\tdo_something_else_to(var);\n\na syntax error.\n\nShould have spotted during the review; sorry.\n\n>\n>> > +#else\n>> > +#define UNLEAK(var)\n>> \n>> I think this should be defined to be something (for example, \"do {}\n>> while (0)\"), at least so that we have compiler errors when UNLEAK(var)\n>> is used incorrectly (for example, without the semicolon) when\n>> SUPPRESS_ANNOTATED_LEAKS is not defined.\n>\n> Seems reasonable.\n\nHmph, I am not so sure about this one. But I agree that the\nsemicolon must go.\n\n\n>\n> I think both are worth doing, but the patch is already in next so we\n> need to do it on top. Do you want to prepare a patch?\n>\n> -Peff\n"},{"id":"328437","messageId":"20170920022838.tlkb5qlmctlstqb6@sigill.intra.peff.net","threadId":"46704","inReplyTo":"xmqqzi9qp2hq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-20T02:28:38Z","receivedAt":"2017-09-20T02:28:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 10:45:05AM +0900, Junio C Hamano wrote:\n\n> >> > +#else\n> >> > +#define UNLEAK(var)\n> >> \n> >> I think this should be defined to be something (for example, \"do {}\n> >> while (0)\"), at least so that we have compiler errors when UNLEAK(var)\n> >> is used incorrectly (for example, without the semicolon) when\n> >> SUPPRESS_ANNOTATED_LEAKS is not defined.\n> >\n> > Seems reasonable.\n> \n> Hmph, I am not so sure about this one. But I agree that the\n> semicolon must go.\n\nI thought we had run into some issues with a compiler or linter\ncomplaining about null statements before. But they _are_ pretty common,\nso maybe I'm mis-remembering (or maybe it was something like the if/else\nconditional you showed earlier).\n\nAt any rate, I think Jonathan's point is that writing:\n\n UNLEAK(foo)\n\nwill silently pass in a normal build, and only much later will somebody\nrun a leak-checking build and see the compile error.\n\nOf course people adding UNLEAK()s are likely to be compiling leak-check\nbuilds to confirm that they've silenced the warning, so maybe it's not\nthat important a case to catch.\n\n-Peff\n"},{"id":"328447","messageId":"xmqqo9q6nebe.fsf@gitster.mtv.corp.google.com","threadId":"46704","inReplyTo":"20170920022838.tlkb5qlmctlstqb6@sigill.intra.peff.net","subject":"Re: [PATCH v2 10/10] add UNLEAK annotation for reducing leak false positives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-20T05:12:37Z","receivedAt":"2017-09-20T05:12:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> At any rate, I think Jonathan's point is that writing:\n>\n> UNLEAK(foo)\n>\n> will silently pass in a normal build, and only much later will somebody\n> run a leak-checking build and see the compile error.\n\nYeah, I think I understand that concern.\n\n\t#if SOME_CONDITION\n\t#define X(y) do_x(y)\n\t#else\n\t#define X(y) /* nothing */\n\t#endif\n\nis something we quite often use, and I just found it a bit unusual\nthat all of a sudden we start to be extra careful only with this\nmacro.\n"}]}