Closed fuzzit-dev[bot] closed 5 years ago
Crasher input: l\xff\xff!:7-6426423770
We've had other OOM crashers of similar type:
:06-68116817
calhost:8-808585085
This is one of the log messages of one of the crashers libFuzzer: out-of-memory (used: 1996Mb; limit: 1984Mb)
. It's worth mentioning that this is relatively low (=249.5 MB) for users who do want to listen on such large range of addresses (~2^32), but it's still kind of wasteful.
Looking at the code that uses this func, I think the fix is simple. This func is used in 8 places all around the project (at commit fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9 at the time of writing). They are:
admin.go -- takes only the first address https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/admin.go#L100-L110
modules/caddyhttp/caddyhttp.go (2 places) -- loop over addresses https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/modules/caddyhttp/caddyhttp.go#L138-L147 https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/modules/caddyhttp/caddyhttp.go#L179-L232
modules/caddyhttp/server.go (3 places; 2 in one func) -- loop over addresses https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/modules/caddyhttp/server.go#L240-L248 https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/modules/caddyhttp/server.go#L253-L277
modules/caddyhttp/reverseproxy/healthchecks.go -- takes only the first one https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/modules/caddyhttp/reverseproxy/healthchecks.go#L105-L132
modules/caddyhttp/reverseproxy/hosts.go -- takes only the first one https://github.com/caddyserver/caddy/blob/fb06c041c4be4eb32f18d54e8e7feff8dd76b0e9/modules/caddyhttp/reverseproxy/hosts.go#L196-L217
I think we can optimize this by doing away with the slice, and replace it with a struct with fields for protocol (e.g. tcp, unix, udp, etc.) as string, host as string, port range start as int, and port range end as int. We can do away with the loop in ParseNetworkAddress
and just return an instance of this struct. Funcs that use only the first address can go ahead withe using the protocol+host+port range start. Funcs that loop over the addresses can change their loop condition to start the index from the port range start to port range end inclusive. If both of these numbers are equal, then it'll go through the loop once, otherwise it'll loop just as many as needed.
How does this sound?
It's worth mentioning that this is relatively low (=249.5 MB) for users who do want to listen on such large range of addresses (~2^32), but it's still kind of wasteful.
This is amazing :joy:
And yes you are right. Great analysis!
I do like your idea a lot, it reduces memory and still allows a large port range.
Regardless, I think we should limit the port range to, say, 64K. That should solve the problems even without needing to use a struct. I'd be happy to review a struct implementation though.
But first, let's at least limit the port range to 64K (can ports go much higher anyway?) before you go into the extra effort. We can discuss the struct in more detail at that time.
According to Wikipedia (too lazy to find the actual RFC):
A port number is a 16-bit unsigned integer, thus ranging from 0 to 65535.
We can limit it to that. I'll send the PR that covers both later today.
The fuzzer been running for more than an hour with no crash yet, as opposed to previous runs that ran into crasher within second~minutes. Considering this resolved.
A new crash was discovered for fuzzing target parse-network-address. Here is a snippet of the log:
More details can be found here
Cheers, Fuzzit Bot