Re: [PATCH 04/33] notes: make get_note return pointer to struct object_id

[Brandon Williams]

On 07/15, René Scharfe wrote:
> Am 30.05.2017 um 19:30 schrieb Brandon Williams:
> > @@ -392,7 +392,7 @@ static int add(int argc, const char **argv, const char 
> > *prefix)
> > const char *object_ref;
> > struct notes_tree *t;
> > unsigned char object[20], new_note[20];
> > -   const unsigned char *note;
> > +   const struct object_id *note;
> > struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> > struct option options[] = {
> > { OPTION_CALLBACK, 'm', "message", , N_("message"),
> 
> In between here, note can be set to NULL...
> 
> > @@ -453,7 +453,7 @@ static int add(int argc, const char **argv, const char 
> > *prefix)
> > sha1_to_hex(object));
> > }
> >   
> > -   prepare_note_data(object, , note);
> > +   prepare_note_data(object, , note->hash);
> 
> ... which we then dereference here.
> 
> > @@ -598,13 +598,13 @@ static int append_edit(int argc, const char **argv, 
> > const char *prefix)
> > t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
> > note = get_note(t, object);
> >   
> > -   prepare_note_data(object, , edit ? note : NULL);
> > +   prepare_note_data(object, , edit && note ? note->hash : NULL);
> 
> Here a NULL check was added; we need a similar one above as well.
> 
> -- >8 --
> Subject: [PATCH] notes: don't access hash of NULL object_id pointer
> 
> Check if note is NULL, as we already do for different purposes a few
> lines above, and pass a NULL pointer to prepare_note_data() in that
> case instead of trying to access the hash member.

Looks good, thanks for catching this!

> 
> Found with Clang's UBSan.
> 
> Signed-off-by: Rene Scharfe 
> ---
> The third parameter of prepare_note_data() could easily be turned into
> an object_id pointer (and it should), but this patch is meant to be a
> minimal fix.
> 
>  builtin/notes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 77573cf1ea..4303848e04 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -456,7 +456,7 @@ static int add(int argc, const char **argv, const char 
> *prefix)
>   oid_to_hex());
>   }
>  
> - prepare_note_data(, , note->hash);
> + prepare_note_data(, , note ? note->hash : NULL);
>   if (d.buf.len || allow_empty) {
>   write_note_data(, new_note.hash);
>   if (add_note(t, , _note, combine_notes_overwrite))
> -- 
> 2.13.3

-- 
Brandon Williams

Re: [PATCH 04/33] notes: make get_note return pointer to struct object_id

[René Scharfe]

Am 30.05.2017 um 19:30 schrieb Brandon Williams:
> @@ -392,7 +392,7 @@ static int add(int argc, const char **argv, const char 
> *prefix)
>   const char *object_ref;
>   struct notes_tree *t;
>   unsigned char object[20], new_note[20];
> - const unsigned char *note;
> + const struct object_id *note;
>   struct note_data d = { 0, 0, NULL, STRBUF_INIT };
>   struct option options[] = {
>   { OPTION_CALLBACK, 'm', "message", , N_("message"),

In between here, note can be set to NULL...

> @@ -453,7 +453,7 @@ static int add(int argc, const char **argv, const char 
> *prefix)
>   sha1_to_hex(object));
>   }
>   
> - prepare_note_data(object, , note);
> + prepare_note_data(object, , note->hash);

... which we then dereference here.

> @@ -598,13 +598,13 @@ static int append_edit(int argc, const char **argv, 
> const char *prefix)
>   t = init_notes_check(argv[0], NOTES_INIT_WRITABLE);
>   note = get_note(t, object);
>   
> - prepare_note_data(object, , edit ? note : NULL);
> + prepare_note_data(object, , edit && note ? note->hash : NULL);

Here a NULL check was added; we need a similar one above as well.

-- >8 --
Subject: [PATCH] notes: don't access hash of NULL object_id pointer

Check if note is NULL, as we already do for different purposes a few
lines above, and pass a NULL pointer to prepare_note_data() in that
case instead of trying to access the hash member.

Found with Clang's UBSan.

Signed-off-by: Rene Scharfe 
---
The third parameter of prepare_note_data() could easily be turned into
an object_id pointer (and it should), but this patch is meant to be a
minimal fix.

 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 77573cf1ea..4303848e04 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -456,7 +456,7 @@ static int add(int argc, const char **argv, const char 
*prefix)
oid_to_hex());
}
 
-   prepare_note_data(, , note->hash);
+   prepare_note_data(, , note ? note->hash : NULL);
if (d.buf.len || allow_empty) {
write_note_data(, new_note.hash);
if (add_note(t, , _note, combine_notes_overwrite))
-- 
2.13.3