Re: [PATCH 3/5] fast-import: clean up pack_data pointer in end_packfile
Print an error before returning when pack_data is NULL ? Otherwise, LGTM On Fri, Aug 22, 2014 at 10:27 PM, Jeff King p...@peff.net wrote: We have a global pointer pack_data pointing to the current pack we have open. Inside end_packfile we have two new pointers, old_p and new_p. The latter points to pack_data, and the former points to the new installed version of the packfile we get when we hand the file off to the regular sha1_file machinery. When then free old_p. Presumably the extra old_p pointer was there so that we could overwrite pack_data with new_p and still free old_p, but we don't do that. We just leave pack_data pointing to bogus memory, and don't overwrite it until we call start_packfile again (if ever). This can cause problems for our die routine, which calls end_packfile to clean things up. If we die at the wrong moment, we can end up looking at invalid memory in pack_data left after the last end_packfile(). Instead, let's make sure we set pack_data to NULL after we free it, and make calling endfile() again with a NULL pack_data a noop (there is nothing to end). We can further make things less confusing by dropping old_p entirely, and moving new_p closer to its point of use. Signed-off-by: Jeff King p...@peff.net --- Noticed while running fast-import under valgrind to debug the next commit. :) fast-import.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fast-import.c b/fast-import.c index d73f58c..f25a4ae 100644 --- a/fast-import.c +++ b/fast-import.c @@ -946,10 +946,12 @@ static void unkeep_all_packs(void) static void end_packfile(void) { - struct packed_git *old_p = pack_data, *new_p; + if (!pack_data) + return; clear_delta_base_cache(); if (object_count) { + struct packed_git *new_p; unsigned char cur_pack_sha1[20]; char *idx_name; int i; @@ -991,10 +993,11 @@ static void end_packfile(void) pack_id++; } else { - close(old_p-pack_fd); - unlink_or_warn(old_p-pack_name); + close(pack_data-pack_fd); + unlink_or_warn(pack_data-pack_name); } - free(old_p); + free(pack_data); + pack_data = NULL; /* We can't carry a delta across packfiles. */ strbuf_release(last_blob.data); -- 2.1.0.346.ga0367b9 -- 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 3/5] fast-import: clean up pack_data pointer in end_packfile
On Mon, Aug 25, 2014 at 10:16:52AM -0700, Ronnie Sahlberg wrote: Print an error before returning when pack_data is NULL ? I don't think so. We call end_packfile in some code paths (like the die handler) as close if it's open. So I think it makes sense for it to be a noop if nothing is open. -Peff -- 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 3/5] fast-import: clean up pack_data pointer in end_packfile
We have a global pointer pack_data pointing to the current pack we have open. Inside end_packfile we have two new pointers, old_p and new_p. The latter points to pack_data, and the former points to the new installed version of the packfile we get when we hand the file off to the regular sha1_file machinery. When then free old_p. Presumably the extra old_p pointer was there so that we could overwrite pack_data with new_p and still free old_p, but we don't do that. We just leave pack_data pointing to bogus memory, and don't overwrite it until we call start_packfile again (if ever). This can cause problems for our die routine, which calls end_packfile to clean things up. If we die at the wrong moment, we can end up looking at invalid memory in pack_data left after the last end_packfile(). Instead, let's make sure we set pack_data to NULL after we free it, and make calling endfile() again with a NULL pack_data a noop (there is nothing to end). We can further make things less confusing by dropping old_p entirely, and moving new_p closer to its point of use. Signed-off-by: Jeff King p...@peff.net --- Noticed while running fast-import under valgrind to debug the next commit. :) fast-import.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/fast-import.c b/fast-import.c index d73f58c..f25a4ae 100644 --- a/fast-import.c +++ b/fast-import.c @@ -946,10 +946,12 @@ static void unkeep_all_packs(void) static void end_packfile(void) { - struct packed_git *old_p = pack_data, *new_p; + if (!pack_data) + return; clear_delta_base_cache(); if (object_count) { + struct packed_git *new_p; unsigned char cur_pack_sha1[20]; char *idx_name; int i; @@ -991,10 +993,11 @@ static void end_packfile(void) pack_id++; } else { - close(old_p-pack_fd); - unlink_or_warn(old_p-pack_name); + close(pack_data-pack_fd); + unlink_or_warn(pack_data-pack_name); } - free(old_p); + free(pack_data); + pack_data = NULL; /* We can't carry a delta across packfiles. */ strbuf_release(last_blob.data); -- 2.1.0.346.ga0367b9 -- 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