Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 18.10.2013 21:09, schrieb Junio C Hamano: Karsten Blees karsten.bl...@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. Moving free() to the callers or caller's callers would make it much more complicated (more places to change). Besides, most callers don't even have a reference to old_ce and simply remove by position. Of course, this doesn't prevent caller's caller's callers to keep a reference to a removed / replaced entry, as found by Thomas. 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. OK, I'll spend some more time analyzing the call hierarchies to see if there are more uses of removed cache_entries. I'll try to post an updated v4 by the end of the week. Karsten -- 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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Karsten Blees karsten.bl...@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));
}
--
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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 18.10.2013 02:42, schrieb Karsten Blees:
Am 17.10.2013 23:07, schrieb Junio C Hamano:
Junio C Hamano gits...@pobox.com writes:
Karsten Blees karsten.bl...@gmail.com writes:
Am 16.10.2013 23:43, schrieb Junio C Hamano:
* kb/fast-hashmap (2013-09-25) 6 commits
- fixup! diffcore-rename.c: simplify finding exact renames
- diffcore-rename.c: use new hash map implementation
- diffcore-rename.c: simplify finding exact renames
- diffcore-rename.c: move code around to prepare for the next patch
- buitin/describe.c: use new hash map implementation
- add a hashtable implementation that supports O(1) removal
I posted a much more complete v3 [1], but somehow missed Jonathan's fixup!
commit.
Thanks; I'll replace the above with v3 and squash the fix-up in.
Interestingly, v3 applied on 'maint' and then merged to 'master'
seems to break t3600 and t7001 with a coredump.
It would conflict with es/name-hash-no-trailing-slash-in-dirs that
has been cooking in 'next', too; the resolution might be trivial but
I didn't look too deeply into it.
I've pushed a rebased version to
https://github.com/kblees/git/commits/kb/hashmap-v3-next
(no changes yet except for Jonathan's fixup in #04 and merge resolution).
The coredumps are caused by my patch #10, which free()s cache_entries when
they are removed, in combination with submodule.c::stage_updated_gitmodules
(5fee9952 submodule.c: add .gitmodules staging helper functions), which
removes a cache_entry, then modifies and re-adds the (now) free()d memory.
Can't we just use add_file_to_cache here (which replaces cache_entries by
creating a copy)?
No objections from my side. Looks like we could also copy the
cache entry just before remove_cache_entry_at() and use that
copy afterwards, but your solution is so much shorter that I
would really like to use it (unless someone more cache-savvy
than me has any objections).
And by the way: this is the last use of remove_cache_entry_at(),
would it make sense to remove that define while at it? Only the
remove_index_entry_at() function it is defined to is currently
used.
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;
- }
- 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));
}
--
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
--
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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 18.10.2013 21:09, schrieb Junio C Hamano:
Karsten Blees karsten.bl...@gmail.com writes:
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?).
Cargo cult programming. I was looking at other code manipulating
the index (as Documentation/technical/api-in-core-index.txt is
rather terse ;-) and concluded I would need to read the possibly
updated st.st_mode, in case updating the config file would have
changed that.
But this warning may want to stay, no?
Of course you are right on this one. All test ran successfully
with this patch, so I think adding one for that warning makes
sense too. And as that is submodule related stuff I volunteer
for fixing all this ;-)
- 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));
--
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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 16.10.2013 23:43, schrieb Junio C Hamano: * kb/fast-hashmap (2013-09-25) 6 commits - fixup! diffcore-rename.c: simplify finding exact renames - diffcore-rename.c: use new hash map implementation - diffcore-rename.c: simplify finding exact renames - diffcore-rename.c: move code around to prepare for the next patch - buitin/describe.c: use new hash map implementation - add a hashtable implementation that supports O(1) removal I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit. Btw., the test suite didn't catch the uninitialized variable, neither on mingw nor linux nor with valgrind. Is there a way to run tests with STACK_POISON or something? [1] http://thread.gmane.org/gmane.comp.version-control.git/235644 -- 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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Karsten Blees karsten.bl...@gmail.com writes: Am 16.10.2013 23:43, schrieb Junio C Hamano: * kb/fast-hashmap (2013-09-25) 6 commits - fixup! diffcore-rename.c: simplify finding exact renames - diffcore-rename.c: use new hash map implementation - diffcore-rename.c: simplify finding exact renames - diffcore-rename.c: move code around to prepare for the next patch - buitin/describe.c: use new hash map implementation - add a hashtable implementation that supports O(1) removal I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit. Thanks; I'll replace the above with v3 and squash the fix-up in. -- 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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Junio C Hamano gits...@pobox.com writes: Karsten Blees karsten.bl...@gmail.com writes: Am 16.10.2013 23:43, schrieb Junio C Hamano: * kb/fast-hashmap (2013-09-25) 6 commits - fixup! diffcore-rename.c: simplify finding exact renames - diffcore-rename.c: use new hash map implementation - diffcore-rename.c: simplify finding exact renames - diffcore-rename.c: move code around to prepare for the next patch - buitin/describe.c: use new hash map implementation - add a hashtable implementation that supports O(1) removal I posted a much more complete v3 [1], but somehow missed Jonathan's fixup! commit. Thanks; I'll replace the above with v3 and squash the fix-up in. Interestingly, v3 applied on 'maint' and then merged to 'master' seems to break t3600 and t7001 with a coredump. It would conflict with es/name-hash-no-trailing-slash-in-dirs that has been cooking in 'next', too; the resolution might be trivial but I didn't look too deeply into it. -- 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: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 17.10.2013 23:07, schrieb Junio C Hamano:
Junio C Hamano gits...@pobox.com writes:
Karsten Blees karsten.bl...@gmail.com writes:
Am 16.10.2013 23:43, schrieb Junio C Hamano:
* kb/fast-hashmap (2013-09-25) 6 commits
- fixup! diffcore-rename.c: simplify finding exact renames
- diffcore-rename.c: use new hash map implementation
- diffcore-rename.c: simplify finding exact renames
- diffcore-rename.c: move code around to prepare for the next patch
- buitin/describe.c: use new hash map implementation
- add a hashtable implementation that supports O(1) removal
I posted a much more complete v3 [1], but somehow missed Jonathan's fixup!
commit.
Thanks; I'll replace the above with v3 and squash the fix-up in.
Interestingly, v3 applied on 'maint' and then merged to 'master'
seems to break t3600 and t7001 with a coredump.
It would conflict with es/name-hash-no-trailing-slash-in-dirs that
has been cooking in 'next', too; the resolution might be trivial but
I didn't look too deeply into it.
I've pushed a rebased version to
https://github.com/kblees/git/commits/kb/hashmap-v3-next
(no changes yet except for Jonathan's fixup in #04 and merge resolution).
The coredumps are caused by my patch #10, which free()s cache_entries when they
are removed, in combination with submodule.c::stage_updated_gitmodules
(5fee9952 submodule.c: add .gitmodules staging helper functions), which
removes a cache_entry, then modifies and re-adds the (now) free()d memory.
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;
- }
- 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));
}
--
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
What's cooking in git.git (Oct 2013, #03; Wed, 16)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. I think I correctly inherited all the topics Jonathan kept track of during my absence (big thanks to Jonathan); if a topic that has been in his tree is missing, please holler. I am chewing through the list backlog but still have a long way to go. Please be patient for the rest of the week. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [New Topics] * fc/styles (2013-10-16) 7 commits - block-sha1/sha1.c: have SP around arithmetic operators - base85.c: have SP around arithmetic operators - archive.c: have SP around arithmetic operators - alloc.c: have SP around arithmetic operators - abspath.c: have SP around arithmetic operators - alias: have SP around arithmetic operators - C: have space around and || operators C coding style fixes. The ones near the tip have not been sent to the list yet (they cover the same kind of style violation as the second one) and I may either send them or drop some of them if they turn out to conflict with other work in flight---I still haven't caught up with the backlog and do not know. * jk/remote-literal-string-leakfix (2013-10-15) 1 commit - remote: do not copy origin string literal Will merge to 'next'. * jk/split-broken-ident (2013-10-15) 2 commits - SQUASH??? remove reverse scan to simplify the logic - split_ident: parse timestamp from end of line Make the fall-back parsing of commit objects with broken author or committer lines more robust to pick up the timestamps. Will merge to 'next', perhaps after dropping the top one. * sg/prompt-svn-remote-fix (2013-10-15) 1 commit - bash prompt: don't use '+=' operator in show upstream code path Bash portability fix. Will merge to 'next'. * sc/doc-howto-dumb-http (2013-10-16) 1 commit - doc/howto: warn about (dumb)http server document being too old Will merge to 'next'. * sg/t3600-nul-sha1-fix (2013-10-16) 1 commit - t3600: fix broken choking git rm test Will merge to 'next'. -- [Stalled] * tr/merge-recursive-index-only (2013-07-07) 3 commits - merge-recursive: -Xindex-only to leave worktree unchanged - merge-recursive: untangle double meaning of o-call_depth - merge-recursive: remove dead conditional in update_stages() Holding until there is a caller to learn from. * jc/ref-excludes (2013-09-03) 2 commits - document --exclude option - revision: introduce --exclude=glob to tame wildcards People often wished a way to tell git log --branches (and git log --remotes --not --branches) to exclude some local branches from the expansion of --branches (similarly for --tags, --all and --glob=pattern). Now they have one. Needs a matching change to rev-parse. * rv/send-email-cache-generated-mid (2013-08-21) 2 commits - git-send-email: Cache generated message-ids, use them when prompting - git-send-email: add optional 'choices' parameter to the ask sub * rj/read-default-config-in-show-ref-pack-refs (2013-06-17) 3 commits - ### DONTMERGE: needs better explanation on what config they need - pack-refs.c: Add missing call to git_config() - show-ref.c: Add missing call to git_config() The changes themselves are probably good, but it is unclear what basic setting needs to be read for which exact operation. Waiting for clarification. $gmane/228294 * jh/shorten-refname (2013-05-07) 4 commits - t1514: refname shortening is done after dereferencing symbolic refs - shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to origin - t1514: Demonstrate failure to correctly shorten refs/remotes/origin/HEAD - t1514: Add tests of shortening refnames in strict/loose mode When remotes/origin/HEAD is not a symbolic ref, rev-parse --abbrev-ref remotes/origin/HEAD ought to show origin, not origin/HEAD, which is fixed with this series (if it is a symbolic ref that points at remotes/origin/something, then it should show origin/something and it already does). Expecting a reroll, as an early part of a larger series. $gmane/225137 * jc/format-patch (2013-04-22) 2 commits - format-patch: --inline-single - format-patch: rename no_inline field A new option to send a single patch to the standard output to be appended at the bottom of a message. I personally have no need for this, but it was easy enough to cobble together. Tests, docs and stripping out more MIMEy stuff are left as exercises to interested parties. * jk/gitweb-utf8 (2013-04-08) 4 commits - gitweb: Fix broken blob action parameters on blob/commitdiff pages - gitweb: Don't append ';js=(0|1)' to external links - gitweb: Make feed title valid utf8 - gitweb: Fix utf8 encoding for blob_plain, blobdiff_plain, commitdiff_plain,
