Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2013, 19:09 UTC
Message-ID: <xmqqfvrywg4y.fsf@gitster.dls.corp.google.com>
In-Reply-To: <526083E7.6080701@gmail.com>

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.

Show 22 quoted lines

> 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?

Show 15 quoted lines

> -       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"));

>  }