From: Junio C Hamano <gitster@pobox•com>
To: Karsten Blees <karsten.blees@gmail•com>
Cc: Jens Lehmann <Jens.Lehmann@web•de>, git@vger•kernel.org
Subject: Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Date: Fri, 18 Oct 2013 12:09:01 -0700 [thread overview]
Message-ID: <xmqqfvrywg4y.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <526083E7.6080701@gmail.com> (Karsten Blees's message of "Fri, 18 Oct 2013 02:42:15 +0200")
Karsten Blees <karsten.blees@gmail•com> writes:
> The coredumps are caused by my patch #10, which free()s
> cache_entries when they are removed, in combination with ...
Looking at that patch, it makes me wonder if remove_index_entry_at()
and replace_index_entry() should be the ones that frees the old
entry in the first place. A caller may already have a ce pointing
at an old entry and use the information from old_ce to update a new
one after it installed it, e.g.
old_ce = ...
new_ce = make_cache_entry(... old_ce->name, ...);
replace_index_entry(... new_ce);
new_ce->ce_mode = old_ce->cd_mode;
free(old_ce);
The same goes for the functions that remove the entry.
But I am probably biased saying this, because in the old days, cache
entries could never be freed (they were carved out of a contiguous
region of memory, mmapped from the index file). These days, we
parse and run ntoh*() on the on-disk cache entries to create in-core
form, and the "cache entries should never be freed" is no longer
true, but I would not be surprised if there are still some code
leftover that relies on "use after free" being safe, leaking unused
cache entries.
Going forward, I do agree with your patch #10 that removal or
replacing that may make an existing entry unreferenced should free
entries that are no longer used, and "use after free" should be
forbidden.
> Can't we just use add_file_to_cache here (which replaces
> cache_entries by creating a copy)?
>
> diff --git a/submodule.c b/submodule.c
> index 1905d75..e388487 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -116,30 +116,7 @@ int remove_path_from_gitmodules(const char *path)
>
> void stage_updated_gitmodules(void)
> {
> - struct strbuf buf = STRBUF_INIT;
> - struct stat st;
> - int pos;
> - struct cache_entry *ce;
> - int namelen = strlen(".gitmodules");
> -
> - pos = cache_name_pos(".gitmodules", namelen);
> - if (pos < 0) {
> - warning(_("could not find .gitmodules in index"));
> - return;
> - }
I think the remainder is (morally) equivalent between the original
and a single "add-file-to-cache" call, and the version after your
"how about this" patch in the message I am responding to looks more
correct (e.g. why does the original lstat after it has read the
file?).
But this warning may want to stay, no?
> - ce = active_cache[pos];
> - ce->ce_flags = namelen;
> - if (strbuf_read_file(&buf, ".gitmodules", 0) < 0)
> - die(_("reading updated .gitmodules failed"));
> - if (lstat(".gitmodules", &st) < 0)
> - die_errno(_("unable to stat updated .gitmodules"));
> - fill_stat_cache_info(ce, &st);
> - ce->ce_mode = ce_mode_from_stat(ce, st.st_mode);
> - if (remove_cache_entry_at(pos) < 0)
> - die(_("unable to remove .gitmodules from index"));
> - if (write_sha1_file(buf.buf, buf.len, blob_type, ce->sha1))
> - die(_("adding updated .gitmodules failed"));
> - if (add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE))
> + if (add_file_to_cache(".gitmodules", 0))
> die(_("staging updated .gitmodules failed"));
> }
next prev parent reply other threads:[~2013-10-18 19:09 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-16 21:43 What's cooking in git.git (Oct 2013, #03; Wed, 16) Junio C Hamano
2013-10-17 9:48 ` Karsten Blees
2013-10-17 20:40 ` Junio C Hamano
2013-10-17 21:07 ` Junio C Hamano
2013-10-18 0:42 ` Karsten Blees
2013-10-18 19:09 ` Junio C Hamano [this message]
2013-10-18 19:52 ` Jens Lehmann
2013-10-18 20:31 ` [PATCH] submodule: don't access the .gitmodules cache entry after removing it Jens Lehmann
2013-10-22 13:13 ` What's cooking in git.git (Oct 2013, #03; Wed, 16) Karsten Blees
2013-10-18 19:37 ` Jens Lehmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqfvrywg4y.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Jens.Lehmann@web$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=karsten.blees@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox