Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 3, 2024
Merged

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

merged 2 commits into from
Sep 3, 2024

Conversation

civodul
Copy link
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
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.

@ethomson
Copy link
Member

Thanks for catching this and reporting it. 🙏

@civodul @ethomson
Apply suggestions from code review
e3597e9 
Co-authored-by: Edward Thomson <ethomson@edwardthomson.com>
@civodul
Copy link
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
Member

ethomson commented Sep 3, 2024

Thanks for this!

@ethomson ethomson merged commit 403a03b into libgit2:main Sep 3, 2024
19 checks passed
@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