Re: [PATCH] parse_object: clear "parsed" when freeing buffers

From: Jeff King <peff@peff.net>
Date: Jan 24, 2013, 07:07 UTC
Message-ID: <20130124070715.GD610@sigill.intra.peff.net>
In-Reply-To: <8988071A-1DF3-463E-8AF9-AE4EA200D786@me.com>

On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:

Show 11 quoted lines

> Several areas of code would free buffers for object structs that
> contained them ("struct tree" and "struct commit"), but without clearing
> the "parsed" flag. parse_object would clear the flag for "struct tree",
> but commits would remain in an invalid state (marked as parsed but with
> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
> invalid objects stay around and can lead to bad behavior.
> 
> In particular, running pickaxe on all refs in a repository with a cached
> textconv could segfault. If the textconv cache ref was evaluated first
> by cmd_log_walk, a subsequent notes_cache_match_validity call would
> dereference NULL.

Just to be sure I understand, what is going on is something like this?

  Log reads all refs, including notes. After showing the notes commit,
  log frees the buffer to save memory.  Later, doing the diff on a
  commit causes us to use the notes_cache mechanism. That will look at
  the commit at the tip of the notes ref, which log has already
  processed. It will call parse_commit, but that will do nothing, as the
  parsed flag is already set, but the commit buffer remains NULL.

I wonder if this could be the source of the segfault here, too:

  http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355

If you have refs pointing to both a submodule and its superproject, a "log --all --submodule -p" might process the submodule's commits first, free their buffers, and then want to show them again as part of the submodule diff. This code in builtin/log.c:

                if (!rev->reflog_info) {
                        /* we allow cycles in reflog ancestry */
                        free(commit->buffer);
                        commit->buffer = NULL;
                }

assumes we will only ever look at a commit once (except for reflog-walking), but submodule diff violates that.

-Peff