Skip to content

http: Initialize ‘on_status’ when using the http-parser backend.#6870

Merged
ethomson merged 2 commits into
libgit2:mainfrom
civodul:fix-uninitialized-http-parser-proxy-settings
Sep 3, 2024
Merged

http: Initialize ‘on_status’ when using the http-parser backend.#6870
ethomson merged 2 commits into
libgit2:mainfrom
civodul:fix-uninitialized-http-parser-proxy-settings

Conversation

@civodul
Copy link
Copy Markdown
Contributor

@civodul civodul commented Aug 28, 2024

Fixes a bug likely introduced in
d396819 (in 1.8.1) whereby proxy_settings.on_status would be left uninitialized when using the ‘http-parser’ backend (-DUSE_HTTP_PARSER=http-parser), eventually leading to a segfault in http_parser_execute. Valgrind would report use of the uninitialized value like so:

   Conditional jump or move depends on uninitialised value(s)
      at 0x50CD533: http_parser_execute (http_parser.c:910)
      by 0x4928504: git_http_parser_execute (httpparser.c:82)
      by 0x4925C42: client_read_and_parse (httpclient.c:1178)
      by 0x4926F27: git_http_client_read_response (httpclient.c:1458)
      by 0x49255FE: http_stream_read (http.c:427)
      by 0x4929B90: git_smart__recv (smart.c:29)
      by 0x492C147: git_smart__store_refs (smart_protocol.c:58)
      by 0x4929F6C: git_smart__connect (smart.c:171)
      by 0x4904DCE: git_remote_connect_ext (remote.c:963)
      by 0x48A15D2: clone_into (clone.c:449)
      by 0x48A15D2: git__clone (clone.c:546)
      by 0x4010E9: main (libgit2-proxy.c:20)

A simple reproducer is this program:

#include <git2.h>
#include <assert.h>
#include <stdio.h>

int
main ()
{
  int err;
  err = git_libgit2_init ();
  assert (err == 1);

  git_clone_options opts;
  err = git_clone_init_options (&opts, GIT_CLONE_OPTIONS_VERSION);
  assert (err == 0);

  opts.fetch_opts.proxy_opts.type = GIT_PROXY_SPECIFIED;
  opts.fetch_opts.proxy_opts.url = "http://example.org/";

  git_repository *repo;
  err = git_clone (&repo, "http://example.org/whatever.git", "/tmp/example",
		   &opts);
  assert (err != 0);

  const git_error *gerr;
  gerr = giterr_last ();
  fprintf (stderr, "last error: %s\n", gerr->message);

  return 0;
}

Note that this bug does not exist when building libgit2 with -DUSE_HTTP_PARSER=llhttp.

@civodul
http: Initialize ‘on_status’ when using the http-parser backend.
ea7e18e 
Fixes a bug likely introduced in
d396819 (in 1.8.1) whereby
‘proxy_settings.on_status’ would be left uninitialized when using the
‘http-parser’ backend, eventually leading to a segfault in
‘http_parser_execute’.  Valgrind would report use of the uninitialized
value like so:

   Conditional jump or move depends on uninitialised value(s)
      at 0x50CD533: http_parser_execute (http_parser.c:910)
      by 0x4928504: git_http_parser_execute (httpparser.c:82)
      by 0x4925C42: client_read_and_parse (httpclient.c:1178)
      by 0x4926F27: git_http_client_read_response (httpclient.c:1458)
      by 0x49255FE: http_stream_read (http.c:427)
      by 0x4929B90: git_smart__recv (smart.c:29)
      by 0x492C147: git_smart__store_refs (smart_protocol.c:58)
      by 0x4929F6C: git_smart__connect (smart.c:171)
      by 0x4904DCE: git_remote_connect_ext (remote.c:963)
      by 0x48A15D2: clone_into (clone.c:449)
      by 0x48A15D2: git__clone (clone.c:546)
      by 0x4010E9: main (libgit2-proxy.c:20)
@civodul civodul force-pushed the fix-uninitialized-http-parser-proxy-settings branch from bbd0d7d to ea7e18e Compare August 29, 2024 11:51
ethomson
ethomson reviewed Aug 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@ethomson ethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of trying to set individual members, maybe we should just memset the whole things to zero (which emulates our prior behavior). I suggest this because the http-parser that we used to use predated the chunk boundaries processing that came from the proxygen fork...? So not all http-parser implementations (including the one that we previously shipped) seem to have this chunk completion callback.

Comment thread src/libgit2/transports/httpparser.c Outdated
Comment thread src/libgit2/transports/httpparser.c Outdated
@ethomson
Copy link
Copy Markdown
Member

ethomson commented Aug 29, 2024

Thanks for catching this and reporting it. 🙏

@civodul
Copy link
Copy Markdown
Contributor Author

civodul commented Aug 29, 2024

Instead of trying to set individual members, maybe we should just memset the whole things to zero (which emulates our prior behavior). I suggest this because the http-parser that we used to use predated the chunk boundaries processing that came from the proxygen fork...? So not all http-parser implementations (including the one that we previously shipped) seem to have this chunk completion callback.

Sure, using memset makes sense to me (I wasn't aware of the multiple http-parser implementations and thought setting individual fields was a stylistic choice).

@ethomson
Copy link
Copy Markdown
Member

ethomson commented Sep 3, 2024

Thanks for this!

@ethomson ethomson merged commit 403a03b into libgit2:main Sep 3, 2024
@ethomson ethomson added the bug label Sep 29, 2024
This was referenced Oct 26, 2024
Closed
Merged
@ervinpopescu ervinpopescu mentioned this pull request Oct 27, 2024
Closed
@BrewTestBot BrewTestBot mentioned this pull request Dec 28, 2024
Merged
@civodul civodul deleted the fix-uninitialized-http-parser-proxy-settings branch May 27, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ethomson ethomson ethomson left review comments

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@civodul @ethomson