Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2015, 22:47 UTC
Message-ID: <xmqqbnedo31z.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqh9o5o3r2.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 9 quoted lines

> David Turner <dturner@twopensource.com> writes:
>
>> On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
>>> I am sorry for being late to the review, I looked into coverity today as Duy
>>> bugged me to fix the memory allocation stuff[1]
>>
>> Thanks. Junio, can you pleas substitute the attached patch instead?
>
> No.  The topic is already in 'next', no?

Yes, the topic is already in 'next'.  A follow-up fix would be good.

The patch didn't apply cleanly on top of 74ec19d^ to replace 74ec19d anyway, so I was about to discard it, but after conflict resolution, the interdiff turns out just these two hunks.

-- >8 --
Subject: pseudoref: check return values from read_ref()
From: David Turner <dturner@twopensource.com>
Date: Wed, 15 Jul 2015 18:05:28 -0400

These codepaths attempt to compare the "expected" current value with the actual current value, but did not check if we successfully read the current value before comparison.

Signed-off-by: David Turner <dturner@twopensource.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * It is likely that we would end up comparing the expected value with
   garbage when the read fails, and the most likely outcome is that
   they do not match and we fail the transaction, which is all fine.

   So in that sense, this is not all that urgent, but it is nice to
   fix it when we know the code is not kosher.

 refs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 522b19b..1db3654 100644
--- a/refs.c
+++ b/refs.c
@@ -2868,7 +2868,9 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
 
 	if (old_sha1) {
 		unsigned char actual_old_sha1[20];
-		read_ref(pseudoref, actual_old_sha1);
+
+		if (read_ref(pseudoref, actual_old_sha1))
+			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			strbuf_addf(err, "Unexpected sha1 when writing %s", pseudoref);
 			rollback_lock_file(&lock);
@@ -2904,7 +2906,8 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
 					       LOCK_DIE_ON_ERROR);
 		if (fd < 0)
 			die_errno(_("Could not open '%s' for writing"), filename);
-		read_ref(pseudoref, actual_old_sha1);
+		if (read_ref(pseudoref, actual_old_sha1))
+			die("could not read ref '%s'", pseudoref);
 		if (hashcmp(actual_old_sha1, old_sha1)) {
 			warning("Unexpected sha1 when deleting %s", pseudoref);
 			rollback_lock_file(&lock);