{"thread":{"id":"34059","subject":"[PATCH v3 0/2] cherry-pick: fix memory leaks","startedAt":"2013-06-07T22:29:26Z","lastAt":"2013-06-09T18:49:44Z","messageCount":16,"participants":["Felipe Contreras","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"219756","messageId":"1370644168-4745-1-git-send-email-felipe.contreras@gmail.com","threadId":"34059","inReplyTo":null,"subject":"[PATCH v3 0/2] cherry-pick: fix memory leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-07T22:29:26Z","receivedAt":"2013-06-07T22:29:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras (2):\n unpack-trees: plug a memory leak\n read-cache: plug a few leaks\n\n read-cache.c | 4 ++++\n unpack-trees.c | 4 +++-\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\n-- \n1.8.3.698.g079b096\n"},{"id":"219757","messageId":"1370644168-4745-2-git-send-email-felipe.contreras@gmail.com","threadId":"34059","inReplyTo":"1370644168-4745-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 1/2] unpack-trees: plug a memory leak","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-07T22:29:27Z","receivedAt":"2013-06-07T22:29:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Before overwriting the destination index, first let's discard its\ncontents.\n\nSigned-off-by: Felipe Contreras \n---\n unpack-trees.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 57b4074..abe2576 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1166,8 +1166,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \to->src_index = NULL;\n \tret = check_updates(o) ? (-2) : 0;\n-\tif (o->dst_index)\n+\tif (o->dst_index) {\n+\t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n+\t}\n \n done:\n \tclear_exclude_list(&el);\n-- \n1.8.3.698.g079b096\n"},{"id":"219758","messageId":"1370644168-4745-3-git-send-email-felipe.contreras@gmail.com","threadId":"34059","inReplyTo":"1370644168-4745-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-07T22:29:28Z","receivedAt":"2013-06-07T22:29:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We are not freeing 'istate->cache' properly.\n\nWe can't rely on 'initialized' to keep track of the 'istate->cache',\nbecause it doesn't really mean it's initialized. So assume it always has\ndata, and free it before overwriting it.\n\nSigned-off-by: Felipe Contreras \n---\n read-cache.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5e30746..a1dd04d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n+\tfree(istate->cache);\n \tistate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));\n \tistate->initialized = 1;\n \n@@ -1512,6 +1513,9 @@ int discard_index(struct index_state *istate)\n \n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tfree(istate->cache[i]);\n+\tfree(istate->cache);\n+\tistate->cache = NULL;\n+\tistate->cache_alloc = 0;\n \tresolve_undo_clear_index(istate);\n \tistate->cache_nr = 0;\n \tistate->cache_changed = 0;\n-- \n1.8.3.698.g079b096\n"},{"id":"219801","messageId":"51B31651.6020307@lsrfire.ath.cx","threadId":"34059","inReplyTo":"1370644168-4745-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-08T11:32:33Z","receivedAt":"2013-06-08T11:32:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.06.2013 00:29, schrieb Felipe Contreras:\n> We are not freeing 'istate->cache' properly.\n>\n> We can't rely on 'initialized' to keep track of the 'istate->cache',\n> because it doesn't really mean it's initialized. So assume it always has\n> data, and free it before overwriting it.\n\nThat sounds a bit backwards to me. If ->initialized doesn't mean that \nthe index state is initialized then something is fishy. Would it make \nsense and be sufficient to set ->initialized in add_index_entry? Or to \nget rid of it and check for ->cache_alloc instead?\n\n> Signed-off-by: Felipe Contreras \n> ---\n> read-cache.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 5e30746..a1dd04d 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const char *path)\n> \tistate->version = ntohl(hdr->hdr_version);\n> \tistate->cache_nr = ntohl(hdr->hdr_entries);\n> \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n> +\tfree(istate->cache);\n> \tistate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));\n> \tistate->initialized = 1;\n\nYou wrote earlier that this change is safe with current callers and that \nit prevents leaks with the following sequence:\n\ndiscard_cache();\n# add entries\nread_cache();\n\nDo we currently have such a call sequence somewhere? Wouldn't that be a \nbug, namely forgetting to call discard_cache before read_cache?\n\nI've added a \"assert(istate->cache_nr == 0);\" a few lines above and the \ntest suite still passed. With the hunk below, ->cache is also always \nNULL and cache_alloc is always 0 at that point. So we don't need that \nfree call there in the cases covered by the test suite at least -- \nbetter leave it out.\n\n> @@ -1512,6 +1513,9 @@ int discard_index(struct index_state *istate)\n>\n> \tfor (i = 0; i < istate->cache_nr; i++)\n> \t\tfree(istate->cache[i]);\n> +\tfree(istate->cache);\n> +\tistate->cache = NULL;\n> +\tistate->cache_alloc = 0;\n> \tresolve_undo_clear_index(istate);\n> \tistate->cache_nr = 0;\n> \tistate->cache_changed = 0;\n\nI still like this part, but also would still recommend to remove the now \ndoubly-outdated comment a few lines below that says \"no need to throw \naway allocated active_cache\". It was valid back when there was only a \nsingle in-memory instance of the index and no istate parameter.\n\nRené\n"},{"id":"219807","messageId":"CAMP44s2Bp5p1211e6Utdch4B+v3J83GCY0_ucG7duakswkb+pg@mail.gmail.com","threadId":"34059","inReplyTo":"51B31651.6020307@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T12:15:30Z","receivedAt":"2013-06-08T12:15:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe\n wrote:\n> Am 08.06.2013 00:29, schrieb Felipe Contreras:\n>\n>> We are not freeing 'istate->cache' properly.\n>>\n>> We can't rely on 'initialized' to keep track of the 'istate->cache',\n>> because it doesn't really mean it's initialized. So assume it always has\n>> data, and free it before overwriting it.\n>\n>\n> That sounds a bit backwards to me. If ->initialized doesn't mean that the\n> index state is initialized then something is fishy. Would it make sense and\n> be sufficient to set ->initialized in add_index_entry?\n\nI don't know.\n\n> Or to get rid of it and check for ->cache_alloc instead?\n\nThat might make sense. I was thinking on renaming 'initialized' to\n'loaded', but I really don't care.\n\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> read-cache.c | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 5e30746..a1dd04d 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate,\n>> const char *path)\n>> istate->version = ntohl(hdr->hdr_version);\n>> istate->cache_nr = ntohl(hdr->hdr_entries);\n>> istate->cache_alloc = alloc_nr(istate->cache_nr);\n>> + free(istate->cache);\n>> istate->cache = xcalloc(istate->cache_alloc,\n>> sizeof(*istate->cache));\n>> istate->initialized = 1;\n>\n>\n> You wrote earlier that this change is safe with current callers and that it\n> prevents leaks with the following sequence:\n>\n> discard_cache();\n> # add entries\n> read_cache();\n>\n> Do we currently have such a call sequence somewhere?\n\nI don't know.\n\n> Wouldn't that be a\n> bug, namely forgetting to call discard_cache before read_cache?\n\nWhy would it be a bug? There's nothing in the API that hints there's a\nproblem with that.\n\n> I've added a \"assert(istate->cache_nr == 0);\" a few lines above and the test\n> suite still passed. With the hunk below, ->cache is also always NULL and\n> cache_alloc is always 0 at that point. So we don't need that free call\n> there in the cases covered by the test suite at least -- better leave it\n> out.\n\nWhy leave it out? If somebody makes the mistake of doing the above\nsequence, would you prefer that we leak?\n\n-- \nFelipe Contreras\n"},{"id":"219817","messageId":"51B32FFD.5070302@lsrfire.ath.cx","threadId":"34059","inReplyTo":"CAMP44s2Bp5p1211e6Utdch4B+v3J83GCY0_ucG7duakswkb+pg@mail.gmail.com","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-08T13:22:05Z","receivedAt":"2013-06-08T13:22:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.06.2013 14:15, schrieb Felipe Contreras:\n> On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe\n> wrote:\n>>> diff --git a/read-cache.c b/read-cache.c\n>>> index 5e30746..a1dd04d 100644\n>>> --- a/read-cache.c\n>>> +++ b/read-cache.c\n>>> @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate,\n>>> const char *path)\n>>> istate->version = ntohl(hdr->hdr_version);\n>>> istate->cache_nr = ntohl(hdr->hdr_entries);\n>>> istate->cache_alloc = alloc_nr(istate->cache_nr);\n>>> + free(istate->cache);\n>>> istate->cache = xcalloc(istate->cache_alloc,\n>>> sizeof(*istate->cache));\n>>> istate->initialized = 1;\n>>\n>>\n>> You wrote earlier that this change is safe with current callers and that it\n>> prevents leaks with the following sequence:\n>>\n>> discard_cache();\n>> # add entries\n>> read_cache();\n>>\n>> Do we currently have such a call sequence somewhere?\n>\n> I don't know.\n>\n>> Wouldn't that be a\n>> bug, namely forgetting to call discard_cache before read_cache?\n>\n> Why would it be a bug? There's nothing in the API that hints there's a\n> problem with that.\n\nA comment before read_index_from says \"remember to discard_cache() \nbefore reading a different cache!\". That is probably a reminder that \nread_index_from does nothing if ->initialized is set. Entries added \nbefore calling read_index_from make up a different cache, however, so I \nthink this comment applies for the call sequence above as well.\n\nOnly read_index_from and add_index_entry allocate ->cache, and only \ndiscard_index frees it, so the two are a triple like malloc, realloc and \nfree.\n\nGranted, these hints are not part of the API -- that looks like a \ndocumentation bug, however.\n\nSide note: I wonder why we need to guard against multiple \nread_index_from calls in a row with ->initialized. Wouldn't it be \neasier to avoid the duplicate calls in the first place? Finding them \nnow might be not so easy, though.\n\n>> I've added a \"assert(istate->cache_nr == 0);\" a few lines above and the test\n>> suite still passed. With the hunk below, ->cache is also always NULL and\n>> cache_alloc is always 0 at that point. So we don't need that free call\n>> there in the cases covered by the test suite at least -- better leave it\n>> out.\n>\n> Why leave it out? If somebody makes the mistake of doing the above\n> sequence, would you prefer that we leak?\n\nLeaking is better than silently cleaning up after a buggy caller because \nit still allows the underlying bug to be found. Even better would be an \nassert, but it's important to make sure it doesn't trigger for existing \nuse cases.\n\nRené\n"},{"id":"219825","messageId":"CAMP44s3K=VtkeCoKqnU9To9YbfO7vph9MsMWtgLWw0n=cYyq5g@mail.gmail.com","threadId":"34059","inReplyTo":"51B32FFD.5070302@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T14:04:31Z","receivedAt":"2013-06-08T14:04:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe\n wrote:\n> Am 08.06.2013 14:15, schrieb Felipe Contreras:\n\n>> Why leave it out? If somebody makes the mistake of doing the above\n>> sequence, would you prefer that we leak?\n>\n> Leaking is better than silently cleaning up after a buggy caller because it\n> still allows the underlying bug to be found.\n\nNo, it doesn't. The pointer is replaced and forever lost. How is\nleaking memory helping anyone to find the bug?\n\n-- \nFelipe Contreras\n"},{"id":"219836","messageId":"51B35414.1090101@lsrfire.ath.cx","threadId":"34059","inReplyTo":"CAMP44s3K=VtkeCoKqnU9To9YbfO7vph9MsMWtgLWw0n=cYyq5g@mail.gmail.com","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-08T15:56:04Z","receivedAt":"2013-06-08T15:56:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.06.2013 16:04, schrieb Felipe Contreras:\n> On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe\n> wrote:\n>> Am 08.06.2013 14:15, schrieb Felipe Contreras:\n>\n>>> Why leave it out? If somebody makes the mistake of doing the above\n>>> sequence, would you prefer that we leak?\n>>\n>> Leaking is better than silently cleaning up after a buggy caller because it\n>> still allows the underlying bug to be found.\n>\n> No, it doesn't. The pointer is replaced and forever lost. How is\n> leaking memory helping anyone to find the bug?\n\nValgrind can tell you where leaked memory was allocated, but not if you \nfree it in the \"wrong\" place.\n\nRené\n"},{"id":"219838","messageId":"CAMP44s3UYCX+DzgnErB=0GdD3w5k2GkNKjv46ZA_NVHm1Z0YLQ@mail.gmail.com","threadId":"34059","inReplyTo":"51B35414.1090101@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T16:53:18Z","receivedAt":"2013-06-08T16:53:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 10:56 AM, René Scharfe\n wrote:\n> Am 08.06.2013 16:04, schrieb Felipe Contreras:\n>\n>> On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe\n>> wrote:\n>>>\n>>> Am 08.06.2013 14:15, schrieb Felipe Contreras:\n>>\n>>\n>>>> Why leave it out? If somebody makes the mistake of doing the above\n>>>> sequence, would you prefer that we leak?\n>>>\n>>>\n>>> Leaking is better than silently cleaning up after a buggy caller because\n>>> it\n>>> still allows the underlying bug to be found.\n>>\n>>\n>> No, it doesn't. The pointer is replaced and forever lost. How is\n>> leaking memory helping anyone to find the bug?\n>\n> Valgrind can tell you where leaked memory was allocated, but not if you free\n> it in the \"wrong\" place.\n\nIt is the correct place to free it. And nobody will ever find it with\nvalgrind, just like nobody has ever tracked down the hundreds of leaks\nin Git that happen reliably 100% of the time, much less the ones that\nhappen rarely.\n\n-- \nFelipe Contreras\n"},{"id":"219842","messageId":"51B36849.3030608@lsrfire.ath.cx","threadId":"34059","inReplyTo":"CAMP44s3UYCX+DzgnErB=0GdD3w5k2GkNKjv46ZA_NVHm1Z0YLQ@mail.gmail.com","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-08T17:22:17Z","receivedAt":"2013-06-08T17:22:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.06.2013 18:53, schrieb Felipe Contreras:\n> On Sat, Jun 8, 2013 at 10:56 AM, René Scharfe\n> wrote:\n>> Am 08.06.2013 16:04, schrieb Felipe Contreras:\n>>\n>>> On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe\n>>> wrote:\n>>>>\n>>>> Am 08.06.2013 14:15, schrieb Felipe Contreras:\n>>>\n>>>\n>>>>> Why leave it out? If somebody makes the mistake of doing the above\n>>>>> sequence, would you prefer that we leak?\n>>>>\n>>>>\n>>>> Leaking is better than silently cleaning up after a buggy caller because\n>>>> it\n>>>> still allows the underlying bug to be found.\n>>>\n>>>\n>>> No, it doesn't. The pointer is replaced and forever lost. How is\n>>> leaking memory helping anyone to find the bug?\n>>\n>> Valgrind can tell you where leaked memory was allocated, but not if you free\n>> it in the \"wrong\" place.\n>\n> It is the correct place to free it. And nobody will ever find it with\n> valgrind, just like nobody has ever tracked down the hundreds of leaks\n> in Git that happen reliably 100% of the time, much less the ones that\n> happen rarely.\n\nWe could argue about freeing it here or adding a discard_index call \nsomewhere else (which seems more natural to me) once we had a call \nsequence that actually causes such a leak. The test suite contains \nnone, so I'd say we need more tests first.\n\nLots of the existing leaks were not worth plugging because the process \nwould end right after freeing the memory. Leaving clean-up to the OS \nwas a conscious design choice, simplifying the code.\n\nWhen such code is reused in a library or run multiple times while it was \noriginally meant to be run only a single time (like with cherry-pick \n--stdin) the original assumption doesn't hold any more and we have a \nproblem.\n\nLet's find and fix those leaks by freeing memory in the right places. \nFreeing memory just in case in places where we can show that no leak is \ntriggered by our test suite doesn't help.\n\nRené\n"},{"id":"219843","messageId":"CAMP44s1ffOUd3DkphHAj8ZmovBazPFdMgtvEptR6kW9+ZMLLjA@mail.gmail.com","threadId":"34059","inReplyTo":"51B36849.3030608@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-08T17:27:55Z","receivedAt":"2013-06-08T17:27:55Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe\n wrote:\n\n> Let's find and fix those leaks by freeing memory in the right places.\n> Freeing memory just in case in places where we can show that no leak is\n> triggered by our test suite doesn't help.\n\nIt helps; it prevents leaks. The real culprit is the bogus API, but I\ndon't see that changing anytime soon, so there are two options when\nsomebody makes a mistake the API allows; leak or don't leak. And you\nseem to prefer the leak, even though it provides absolutely no\nadvantage.\n\n-- \nFelipe Contreras\n"},{"id":"219867","messageId":"51B3E44C.4030304@lsrfire.ath.cx","threadId":"34059","inReplyTo":"CAMP44s1ffOUd3DkphHAj8ZmovBazPFdMgtvEptR6kW9+ZMLLjA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-09T02:11:24Z","receivedAt":"2013-06-09T02:11:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.06.2013 19:27, schrieb Felipe Contreras:\n> On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe\n> wrote:\n>\n>> Let's find and fix those leaks by freeing memory in the right places.\n>> Freeing memory just in case in places where we can show that no leak is\n>> triggered by our test suite doesn't help.\n>\n> It helps; it prevents leaks. The real culprit is the bogus API, but I\n> don't see that changing anytime soon, so there are two options when\n> somebody makes a mistake the API allows; leak or don't leak. And you\n> seem to prefer the leak, even though it provides absolutely no\n> advantage.\n\nIt covers up bugs, which may seem helpful, but isn't, because it's \nbetter to fix the actual mistake.\n\nWhat would be a better API? Making discard_index free the array is a \ngood first step; what else is bogus?\n\nRené\n"},{"id":"219870","messageId":"CAMP44s0RqtoP8iHZ+rEqPDKSLxZLESS8qKFhb2vzSd7-mtKreQ@mail.gmail.com","threadId":"34059","inReplyTo":"51B3E44C.4030304@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-09T02:25:16Z","receivedAt":"2013-06-09T02:25:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe\n wrote:\n> Am 08.06.2013 19:27, schrieb Felipe Contreras:\n>\n>> On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe\n>> wrote:\n>>\n>>> Let's find and fix those leaks by freeing memory in the right places.\n>>> Freeing memory just in case in places where we can show that no leak is\n>>> triggered by our test suite doesn't help.\n>>\n>>\n>> It helps; it prevents leaks. The real culprit is the bogus API, but I\n>> don't see that changing anytime soon, so there are two options when\n>> somebody makes a mistake the API allows; leak or don't leak. And you\n>> seem to prefer the leak, even though it provides absolutely no\n>> advantage.\n>\n> It covers up bugs,\n\nIt doesn't. I thought you already silently agreed that nobody would\never find that leak, as they haven't found the hundreds of leaks that\nplague Git's code.\n\n> What would be a better API? Making discard_index free the array is a good\n> first step; what else is bogus?\n\n'initialized' for starters; it should be renamed to 'loaded' or\nremoved, but removing it would require many more changes to make sure\nwe don't load twice. Also, when loading cache entries, it might make\nsense to check if there's already entries that have not been\npreviously discarded properly.\n\nIn the meantime, just in case, the only sane thing to do is free the\nentries rather than leak.\n\nThat being said I'm not interested in this patch any more. The patch\nis good yet after three tries and countless arguments it's still not\napplied, nor is there any sign of getting there.\n\n-- \nFelipe Contreras\n"},{"id":"220044","messageId":"51B4BD9F.6070107@lsrfire.ath.cx","threadId":"34059","inReplyTo":"CAMP44s0RqtoP8iHZ+rEqPDKSLxZLESS8qKFhb2vzSd7-mtKreQ@mail.gmail.com","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-06-09T17:38:39Z","receivedAt":"2013-06-09T17:38:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.06.2013 04:25, schrieb Felipe Contreras:\n> On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe\n> wrote:\n>> Am 08.06.2013 19:27, schrieb Felipe Contreras:\n>>\n>>> On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe\n>>> wrote:\n>>>\n>>>> Let's find and fix those leaks by freeing memory in the right places.\n>>>> Freeing memory just in case in places where we can show that no leak is\n>>>> triggered by our test suite doesn't help.\n>>>\n>>>\n>>> It helps; it prevents leaks. The real culprit is the bogus API, but I\n>>> don't see that changing anytime soon, so there are two options when\n>>> somebody makes a mistake the API allows; leak or don't leak. And you\n>>> seem to prefer the leak, even though it provides absolutely no\n>>> advantage.\n>>\n>> It covers up bugs,\n>\n> It doesn't. I thought you already silently agreed that nobody would\n> ever find that leak, as they haven't found the hundreds of leaks that\n> plague Git's code.\n\nNah, I explained non-silently that leakage was a design decision for \nshort-running commands that allocate memory, use it and exit. Reusing \nsuch code without freeing allocated memory between runs explicitly turns \na \"good\" leak into a \"bad\" one, as we saw with cherry-pick --stdin.\n\n>> What would be a better API? Making discard_index free the array is a good\n>> first step; what else is bogus?\n>\n> 'initialized' for starters; it should be renamed to 'loaded' or\n> removed, but removing it would require many more changes to make sure\n> we don't load twice. Also, when loading cache entries, it might make\n> sense to check if there's already entries that have not been\n> previously discarded properly.\n\nAdding diagnostics that help find leaks is a good idea.\n\nSo, from reading the code, this sequence is OK:\n\n\tdiscard_cache()\t\t// defined starting point\n\tread_cache()\t\t// reads the cache\n\tread_cache()\t\t// does nothing\n\nAnd I guess this one is not OK:\n\n\tdiscard_cache()\t\t// defined starting point\n\tadd_index_entry()\t// add single entry\n\tread_cache()\t\t// currently leaks, should warn/die\n\nAny more sequences that we need to guard against, or counterexamples?\n\n> In the meantime, just in case, the only sane thing to do is free the\n> entries rather than leak.\n\nI consider not plugging a leak which we don't know how to trigger with \nexisting code even more sane. Yay, circles! ;-)\n\n> That being said I'm not interested in this patch any more. The patch\n> is good yet after three tries and countless arguments it's still not\n> applied, nor is there any sign of getting there.\n\nLet's take it step by step: Once the known leak is plugged we can worry \nabout the unknown ones. I'll send small patches.\n\nRené\n"},{"id":"220085","messageId":"CAMP44s0xQajOE=w-j4u=sQF71U9CyKj48ZqJ6s4O7zmUTFa__Q@mail.gmail.com","threadId":"34059","inReplyTo":"51B4BD9F.6070107@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-06-09T18:27:00Z","receivedAt":"2013-06-09T18:27:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sun, Jun 9, 2013 at 12:38 PM, René Scharfe\n wrote:\n> Am 09.06.2013 04:25, schrieb Felipe Contreras:\n\n>> It doesn't. I thought you already silently agreed that nobody would\n>> ever find that leak, as they haven't found the hundreds of leaks that\n>> plague Git's code.\n>\n> Nah, I explained non-silently that leakage was a design decision for\n> short-running commands that allocate memory, use it and exit. Reusing such\n> code without freeing allocated memory between runs explicitly turns a \"good\"\n> leak into a \"bad\" one, as we saw with cherry-pick --stdin.\n\ngit cherry-pick for multiple commits is already there, such a leak\nwould be a bad one, and nobody would find it. Valgrind doesn't know\nabout your concept of \"good\" leaks, all that you see is tons of leaks.\n\n> Any more sequences that we need to guard against, or counterexamples?\n\nI don't know.\n\n>> In the meantime, just in case, the only sane thing to do is free the\n>> entries rather than leak.\n>\n> I consider not plugging a leak which we don't know how to trigger with\n> existing code even more sane. Yay, circles! ;-)\n\nThere's absolutely no benefits to leaving the potential leak.\n\n> Let's take it step by step: Once the known leak is plugged we can worry\n> about the unknown ones. I'll send small patches.\n\nGo ahead. I'm not interested any more.\n\n-- \nFelipe Contreras\n"},{"id":"220094","messageId":"7v8v2jqfuf.fsf@alter.siamese.dyndns.org","threadId":"34059","inReplyTo":"51B32FFD.5070302@lsrfire.ath.cx","subject":"Re: [PATCH v3 2/2] read-cache: plug a few leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-09T18:49:44Z","receivedAt":"2013-06-09T18:49:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> A comment before read_index_from says \"remember to discard_cache()\n> before reading a different cache!\". That is probably a reminder that\n> read_index_from does nothing if ->initialized is set. Entries added\n> before calling read_index_from make up a different cache, however, so\n> I think this comment applies for the call sequence above as well.\n\nYes, you can lose the probably from there. Back when he comment was\nadded at 8fd2cb406917 (Extract helper bits from c-merge-recursive\nwork, 2006-07-25), there was only one in-core index, and checking\nactive_cache or cache_mmap is not NULL was a way to say \"Have we\nloaded from $GIT_DIR/index already? If so, there is nothing more to\ndo\".\n\nLater unpack_trees() added a way to populate the index not by\nreading from $GIT_DIR/index and the original \"Has file, hence is\nloaded, so ignore read_cache()\" caused issues. A discussion was in \n\n http://thread.gmane.org/gmane.comp.version-control.git/93260/focus=93469\n\nwhich brought the \"initialized\" bit in, which lead to 913e0e99b6a6\n(unpack_trees(): protect the handcrafted in-core index from\nread_cache(), 2008-08-23).\n\n> Side note: I wonder why we need to guard against multiple\n> read_index_from calls in a row with ->initialized. Wouldn't it be\n> easier to avoid the duplicate calls in the first place? Finding them\n> now might be not so easy, though.\n"}]}