On Tue, 2015-08-11 at 15:47 -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > David Turner <dturner@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxx>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
>
> * 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);
>
LGTM.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html