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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2013, 00:25 UTC
Message-ID: <7vfw1rfmw2.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1C90CE32-F559-4E76-915E-93642F614552@me.com>

Jonathon Mah <jmah@me.com> writes:

Show 9 quoted lines

> No, I haven't audited the code paths (I have only the loosest
> familiarity with the source). Indeed, I found that clearing the
> 'parsed' flag in fsck.c (traverse_one_object()) is incorrect and
> causes test failures.
>
> With the object cache, isn't modifying the object unsafe in
> general? Instead of auditing code paths, it's now necessary to
> audit _all_ code that uses "struct object", which seems
> infeasible.

The object layer was designed with "run one thing and one thing well, and then let the _exit(2) take care of the clean-up" model in mind; modifying the object, e.g. updating commit->parents list, in-core by revision traversal machinery is very much within the scope of its intended uses.

> I'm just trying to fix the segfault demonstrated in the test
> attached to the patch.

Can offending readers that dereference NULL without checking if buffer has been freed be updated so that they read_sha1_file(), read the contents from the result returned from the function (instead of reading from .buffer), and free the memory when they are done?

That would be a fix that would be very much isolated and easy to audit.