-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
libevent: Fuzzing Coverage Expansion #11257
Conversation
viktoriia-lsg is a new contributor to projects/libevent. The PR must be approved by known contributors before it can be merged. The past contributors are: fanquake, azat, inferno-chromium, devtty1er, Google-Autofuzz |
|
||
int use_pair = data[0] % 2; | ||
int options1 = data[1] % 16; | ||
int options2 = data[2] % 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use data_provider
here to generate the ints instead?
static struct timeval cfg_tick = { static_cast<__time_t>(*(uint32_t*)data), static_cast<__suseconds_t>(*(uint32_t*)(data+1))}; | ||
conn_bucket_cfg = ev_token_bucket_cfg_new( | ||
*(uint32_t *)data, *(uint32_t *)(data + 1), *(uint32_t *)(data + 2), | ||
*(uint32_t *)(data + 3), &cfg_tick); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the above lines could you rely on data consumed by data_provider
instead?
bufferevent_setwatermark(bev2, EV_WRITE | EV_READ, *(size_t *)data, | ||
*(size_t *)(data + 1)); | ||
bufferevent_getwatermark(bev2, EV_WRITE | EV_READ, (size_t *)(data + 1), | ||
(size_t *)data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the above lines could you rely on data consumed by data_provider
instead?
@DavidKorczynski I've added a data_provider for the specified cases, please take a look. The tests failed due to a valid memory leak in evbuffer_add_cb. It looks like evbuffer_cb_entry is not freed later in the libevent code. |
Hi @DavidKorczynski. Could you please take a look? |
Apologies for the delay @viktoriia-lsg -- I'll review this Tuesday. |
|
||
/*bufferevent_filter_new*/ | ||
bev1 = bufferevent_filter_new(bev1, NULL, NULL, options1, NULL, NULL); | ||
bev2 = bufferevent_filter_new(bev2, NULL, NULL, options2, NULL, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the leaks from the CI are valid leaks. I think they happen because you re-assign bev2
here (previously assigned on line 57) and that overwrites a pointer to some dynamic memory. Thus, a pointer is left hanging -- I think the fuzzer is missing some cleanup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirm that the following seems to fix it:
diff --git a/projects/libevent/bufferevent_fuzzer.cc b/projects/libevent/bufferevent_fuzzer.cc
index c3bde3138..9f5e65e7c 100644
--- a/projects/libevent/bufferevent_fuzzer.cc
+++ b/projects/libevent/bufferevent_fuzzer.cc
@@ -60,6 +60,14 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
bev1 = bufferevent_socket_new(base, -1, options1);
bev2 = bufferevent_socket_new(base, -1, options2);
}
+ if (bev1) {
+ bufferevent_free(bev1);
+ }
+ if (bev2) {
+ bufferevent_free(bev2);
+ }
+ bev1 = NULL;
+ bev2 = NULL;
/*bufferevent_filter_new*/
bev1 = bufferevent_filter_new(bev1, NULL, NULL, options1, NULL, NULL);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavidKorczynski I really appreciate your help. Indeed, it was an issue with hanging pointers on lines 65 and 66. I fixed the fuzz target and now it works fine, as well I resolved the issue with double free.
I did not use your fix, because bufferevent_filter_new function accepts a non-NULL bufferevent object, but it helped me to realize where the problem was. In details, I've added two bufferevent objects to be assigned from bufferevent_filter_new function and set conn_bucket_cfg to NULL to fix potential double free. Please approve when you have time. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I looked through this and left my 50 cents.
sprintf(hostsFilename, "/tmp/hosts.%d", getpid()); | ||
fp = fopen(hostsFilename, "wb"); | ||
if (!fp) { | ||
evdns_base_free(dns, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misses unlink(resolvFilename);
.
I guess it is better to rewrite it to goto
if (use_pair == 0) { | ||
if (bufferevent_pair_new(base, options1, pair) == -1) { | ||
event_base_free(base); | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not goto cleanup
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it to make it simpler. If I use "goto cleanup", the code will go through 4 unnecessary if statements.
bev2 = bufferevent_socket_new(base, -1, options2); | ||
} | ||
|
||
/*bufferevent_filter_new*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it worth to make this under condition, like use_pair
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify it? Do you mean to create bufferevent
with bufferevent_socket_new
function in another if
statement? In such case, bufferevent_filter_new
will throw an error, as it is waiting to receive a valid bufferevent
as the first param.
UPD. I got it and added use_filter param, so not all bufferevent will be with filters.
size_t int4 = data_provider.ConsumeIntegral<size_t>(); | ||
|
||
int use_pair = int1 % 2; | ||
int options1 = int2 % 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using bufferevent_free
here explicitly for filters, so you cannot use BEV_OPT_CLOSE_ON_FREE
bufferevent_priority_set(bev3, options2); | ||
|
||
/*set rate limits*/ | ||
bufferevent_set_rate_limit(bev3, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also make it conditionally?
bufferevent_remove_from_rate_limit_group(bev4); | ||
|
||
/*watermarks*/ | ||
bufferevent_setwatermark(bev4, EV_WRITE | EV_READ, int1, int2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here as well, let's add some randomization, watermarks be applied or not.
fstat(fd, &st); | ||
|
||
struct evbuffer *buf = evbuffer_new(); | ||
evbuffer_add_file(buf, fd, 0, st.st_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't expect any problems here, since it just will read the whole file or mmap it.
Let's add evbuffer_set_flags
at least (EVBUF_FS_CLOSE_ON_FREE
and friends)
@viktoriia-lsg thank you! |
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257
evbuffer_add_file(buf, fd, 0, st.st_size); | ||
|
||
fclose(fp); | ||
close(fd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fd can been closed by fclose
already, this close
should be removed.
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: better check (found by CI for windows)
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test v3: fix for windows (_get_osfhandle() crashes when called on closed fd)
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test v3: fix for windows (_get_osfhandle() crashes when called on closed fd) v4: fix for EVENT__DISABLE_MM_REPLACEMENT
Found by oss-fuzz, after coverage had been improved in google/oss-fuzz#11257 v2: adjust test v3: fix for windows (_get_osfhandle() crashes when called on closed fd) v4: fix for EVENT__DISABLE_MM_REPLACEMENT
Hi! This pull request expands fuzzing coverage of libevent by adding 3 new fuzz targets.