Re: [PATCH GSoC 2/5] fetch-object-info: parse type from server response

[snip]
> >> @@ -148,6 +154,10 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar
> >>                             object_info_values.items[0].string,
> >>                             object_info_values.items[size_index + 1].string);
> >>
> >> +               if (type_index >= 0)
> >> +                       *object_info_data[i].typep =
> >> +                               type_from_string(object_info_values.items[type_index + 1].string);
> >> +
> >>                 string_list_clear(&object_info_values, 0);
> >
> > Is there a risk of an out-of-bounds array access here if the server
> > responds with a truncated or malformed packet?
> >
> > If object_info_values.nr <= type_index + 1, this will segfault.
>
> This shouldn't be a possible case because of:
>
> fetch_object_info()
>
>         for (size_t i = 0; i < args->object_info_options->nr; i++) {
>
>                 [snip]
>
>                 } else if (!strcmp(reader->line, "type")) {
>                         type_index = (int)i;
>
>                 [snip]
>
> type_index is set based of the range of object_info_options->nr so:
>   type_index < object_info_options->nr
>
> and a few lines below:
>
>         if (args->object_info_options->nr + 1 != object_info_values.nr)
>                 die("object-info: unexpected number of attributes: %s",
>                     reader->line);
>
> so we also know that type_index + 1 < object_info_values.nr.
> After that we get to those lines that this patch introduced:
>
>
>  +               if (type_index >= 0)
>  +                       *object_info_data[i].typep =
>  +                               type_from_string(object_info_values.items[type_index + 1].string);
>
>  And because type_index + 1 < object_info_values.nr we can be sure that
>  this cannot segfault once we reach this code.

Makes sense to me.

> >
> > If there isn't a bounds check slightly higher up in this loop, we should
> > add one. Either way, we should definitely add a test using a mocked
> > server response (e.g., via test-tool pkt-line) to ensure the client
> > gracefully dies with a protocol error rather than segfaulting when it
> > receives a malformed packet.
>
> Ok, that's sounds a good test, I think there's none where a malicious
> server is simulated, in part because I don't know how and I think I
> haven't seen a test that does that yet.
> test-tool and pkt-line are used for the opposite: simulating the
> client to test the real server.
>
> I'll see what I can do about it.

Yeah, I wouldn't recommend breaking your back for it though. We
already have tests exploring the happy paths, so something that
simply verifies our expectations for error paths (printing an empty
string in this case) should be good enough.