Re: [PATCH v2 1/3] read-cache: plug a few leaks
On Thu, May 30, 2013 at 10:13 AM, René Scharfe rene.scha...@lsrfire.ath.cx wrote: Am 30.05.2013 15:34, schrieb Felipe Contreras: We don't free 'istate-cache' properly. Apparently 'initialized' doesn't really mean initialized, but loaded, or rather 'not-empty', and the cache can be used even if it's not 'initialized', so we can't rely on this variable to keep track of the 'istate-cache'. So assume it always has data, and free it before overwriting it. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- read-cache.c | 4 1 file changed, 4 insertions(+) diff --git a/read-cache.c b/read-cache.c index 04ed561..e5dc96f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path) istate-version = ntohl(hdr-hdr_version); istate-cache_nr = ntohl(hdr-hdr_entries); istate-cache_alloc = alloc_nr(istate-cache_nr); + free(istate-cache); With that change, callers of read_index_from need to set -cache to NULL for uninitialized (on-stack) index_state variables. They only had to set -initialized to 0 before in that situation. It this chunk safe for all existing callers? Shouldn't the same free in discard_index (added below) be enough? It would be enough if every discard_cache() is not followed by a read_cache() after adding entries. I was adding a init_index() helper, but it turns out only very few places initialize the index, and all of them zero the structure (or declare it so it's zeroed on load), so I think this change is safe like that. Also, I don't see any place manually doing initialize=0. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] read-cache: plug a few leaks
We don't free 'istate-cache' properly. Apparently 'initialized' doesn't really mean initialized, but loaded, or rather 'not-empty', and the cache can be used even if it's not 'initialized', so we can't rely on this variable to keep track of the 'istate-cache'. So assume it always has data, and free it before overwriting it. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- read-cache.c | 4 1 file changed, 4 insertions(+) diff --git a/read-cache.c b/read-cache.c index 04ed561..e5dc96f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path) istate-version = ntohl(hdr-hdr_version); istate-cache_nr = ntohl(hdr-hdr_entries); istate-cache_alloc = alloc_nr(istate-cache_nr); + free(istate-cache); istate-cache = xcalloc(istate-cache_alloc, sizeof(struct cache_entry *)); istate-initialized = 1; @@ -1510,6 +1511,9 @@ int discard_index(struct index_state *istate) for (i = 0; i istate-cache_nr; i++) free(istate-cache[i]); + free(istate-cache); + istate-cache = NULL; + istate-cache_alloc = 0; resolve_undo_clear_index(istate); istate-cache_nr = 0; istate-cache_changed = 0; -- 1.8.3.rc3.312.g47657de -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] read-cache: plug a few leaks
Am 30.05.2013 15:34, schrieb Felipe Contreras: We don't free 'istate-cache' properly. Apparently 'initialized' doesn't really mean initialized, but loaded, or rather 'not-empty', and the cache can be used even if it's not 'initialized', so we can't rely on this variable to keep track of the 'istate-cache'. So assume it always has data, and free it before overwriting it. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- read-cache.c | 4 1 file changed, 4 insertions(+) diff --git a/read-cache.c b/read-cache.c index 04ed561..e5dc96f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path) istate-version = ntohl(hdr-hdr_version); istate-cache_nr = ntohl(hdr-hdr_entries); istate-cache_alloc = alloc_nr(istate-cache_nr); + free(istate-cache); With that change, callers of read_index_from need to set -cache to NULL for uninitialized (on-stack) index_state variables. They only had to set -initialized to 0 before in that situation. It this chunk safe for all existing callers? Shouldn't the same free in discard_index (added below) be enough? istate-cache = xcalloc(istate-cache_alloc, sizeof(struct cache_entry *)); istate-initialized = 1; @@ -1510,6 +1511,9 @@ int discard_index(struct index_state *istate) for (i = 0; i istate-cache_nr; i++) free(istate-cache[i]); + free(istate-cache); + istate-cache = NULL; + istate-cache_alloc = 0; resolve_undo_clear_index(istate); istate-cache_nr = 0; istate-cache_changed = 0; I was preparing a similar change, looks good. There is a comment at the end of discard_index() that becomes wrong due to that patch, though -- better remove it as well. It was already outdated as it mentioned active_cache, while the function can be used with any index_state. diff --git a/read-cache.c b/read-cache.c index e5dc96f..0f868af 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1522,8 +1522,6 @@ int discard_index(struct index_state *istate) free_name_hash(istate); cache_tree_free((istate-cache_tree)); istate-initialized = 0; - - /* no need to throw away allocated active_cache */ return 0; } -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] read-cache: plug a few leaks
On Thu, May 30, 2013 at 10:13 AM, René Scharfe rene.scha...@lsrfire.ath.cx wrote: Am 30.05.2013 15:34, schrieb Felipe Contreras: We don't free 'istate-cache' properly. Apparently 'initialized' doesn't really mean initialized, but loaded, or rather 'not-empty', and the cache can be used even if it's not 'initialized', so we can't rely on this variable to keep track of the 'istate-cache'. So assume it always has data, and free it before overwriting it. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- read-cache.c | 4 1 file changed, 4 insertions(+) diff --git a/read-cache.c b/read-cache.c index 04ed561..e5dc96f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path) istate-version = ntohl(hdr-hdr_version); istate-cache_nr = ntohl(hdr-hdr_entries); istate-cache_alloc = alloc_nr(istate-cache_nr); + free(istate-cache); With that change, callers of read_index_from need to set -cache to NULL for uninitialized (on-stack) index_state variables. They only had to set -initialized to 0 before in that situation. It this chunk safe for all existing callers? Shouldn't the same free in discard_index (added below) be enough? We can remove that line, but then if some code does this: discard_cache(); # add entries read_cache(); There will be a memory leak. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
