Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Ensure host offset & length are initialized - fuzzer found possible leaks #440

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zzo
Copy link

@zzo zzo commented Jul 23, 2018

No description provided.

@ploxiln
Copy link
Contributor

ploxiln commented Jul 23, 2018

this should be taken care of already by

http_parser_url_init(struct http_parser_url *u) {
  memset(u, 0, sizeof(*u));
}

@zzo
Copy link
Author

zzo commented Jul 23, 2018

cool so http_parser_parse_url should call that?

@ploxiln
Copy link
Contributor

ploxiln commented Jul 23, 2018

The user of the library should call that before calling http_parser_parse_url(). Or, alternatively, the struct http_parser_url might be a member of a larger struct that was memset() all at once, or was allocated with calloc(), in which case the redundant clearing could be skipped ...

If there's any "bug" here, it's that http_parser_parse_url() is only very briefly mentioned and is not really documented ... just the existence of http_parser_url_init(struct http_parser_url *u) is there to imply you should initialize the struct.

@zzo
Copy link
Author

zzo commented Jul 23, 2018

ok cool of the 3 references to it being called only 1 in test.c doesn't do this: https://github.com/nodejs/node/blob/bc690e9ef52bebc34cad7ddb40e74472bcb272ca/deps/http_parser/test.c#L2614
not a big deal! still it couldn't hurt to 0 out the struct in case someone forgets to initialize it no? Otherwise just close & ignore this - thanks! Or I can create another PR that just calls http_parser_url_init from http_parser_parse_url - is there ever a case where you don't want to do this (except the case that maybe the caller has already done so)?

@ploxiln
Copy link
Contributor

ploxiln commented Jul 23, 2018

Looks like a bug in the test to me.
I'm not a maintainer though, just trying to help clarify the situation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants