Skip to content
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

Fix potential Null pointer dereference in test-ratelim.c #1602

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix potential Null pointer dereference in test-ratelim.c #1602

wants to merge 3 commits into from

Conversation

icy17
Copy link
Contributor

@icy17 icy17 commented Apr 10, 2024

No description provided.

azat
azat requested changes Apr 15, 2024
View reviewed changes
test/test-ratelim.c Outdated
@@ -195,10 +195,12 @@ echo_listenercb(struct evconnlistener *listener, evutil_socket_t newsock,
struct bufferevent *bev;

bev = bufferevent_socket_new(base, newsock, flags);
assert(bev);
Copy link
Member

Choose a reason for hiding this comment

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

This is not good error handler, let's close fd and write an error

test/test-ratelim.c Outdated Show resolved Hide resolved
test/test-ratelim.c Outdated
@@ -449,10 +451,12 @@ test_ratelimiting(void)
ms100_common = event_base_init_common_timeout(base, &tv);

periodic_level_check = event_new(base, -1, EV_PERSIST, check_group_bucket_levels_cb, NULL);
assert(periodic_level_check);
Copy link
Member

Choose a reason for hiding this comment

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

Should have cleanup error handler

test/test-ratelim.c Outdated
event_add(periodic_level_check, ms100_common);

if (cfg_group_drain && ratelim_group) {
group_drain_event = event_new(base, -1, EV_PERSIST, group_drain_cb, NULL);
assert(group_drain_event);
Copy link
Member

Choose a reason for hiding this comment

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

Same

@icy17
Copy link
Contributor Author

icy17 commented Apr 15, 2024

OK, I'll fix them

@icy17
Copy link
Contributor Author

icy17 commented Apr 22, 2024

I just move some free to cleanup, and I'm not sure if the modifications are correct.

@azat
Copy link
Member

azat commented Apr 29, 2024

and I'm not sure if the modifications are correct.

You can run make test locally and look at our CI:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==3801==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x7fe058787ba3 bp 0x7ffc8bf25550 sp 0x7ffc8bf254e0 T0)
==3801==The signal is caused by a READ memory access.
==3801==Hint: address points to the zero page.
    #0 0x7fe058787ba3 in bufferevent_rate_limit_group_decrement_read /home/runner/work/libevent/libevent/bufferevent_ratelim.c:1049:2
    #1 0x557e063d31e8 in group_drain_cb /home/runner/work/libevent/libevent/test/test-ratelim.c:271:2
    #2 0x7fe0587af03e in event_persist_closure /home/runner/work/libevent/libevent/event.c:1659:2
    #3 0x7fe0587ac9cf in event_process_active_single_queue /home/runner/work/libevent/libevent/event.c:1718:4
    #4 0x7fe05879d261 in event_process_active /home/runner/work/libevent/libevent/event.c:1819:9
    #5 0x7fe058799d1a in event_base_loop /home/runner/work/libevent/libevent/event.c:2068:12
    #6 0x7fe058798a66 in event_base_dispatch /home/runner/work/libevent/libevent/event.c:1853:10
    #7 0x557e063d20a4 in test_ratelimiting /home/runner/work/libevent/libevent/test/test-ratelim.c:492:3
    #8 0x557e063d0802 in main /home/runner/work/libevent/libevent/test/test-ratelim.c:715:9
    #9 0x7fe058429d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
    #10 0x7fe058429e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: c289da5071a3399de893d2af81d6a30c62646e1e)
    #11 0x557e06312664 in _start (/home/runner/work/libevent/libevent/build/bin/test-ratelim+0x21664) (BuildId: b9c[75](https://github.com/libevent/libevent/actions/runs/8779810435/job/24088531147#step:7:76)ba8e0fc6187d4a0d34357e3c53394a8af81)

azat
azat reviewed Apr 29, 2024
View reviewed changes
test/test-ratelim.c Outdated Show resolved Hide resolved
azat
azat reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

Apart from one comment looks ok

test/test-ratelim.c
@@ -256,8 +269,10 @@ check_group_bucket_levels_cb(evutil_socket_t fd, short events, void *arg)
static void
group_drain_cb(evutil_socket_t fd, short events, void *arg)
{
bufferevent_rate_limit_group_decrement_read(ratelim_group, cfg_group_drain);
bufferevent_rate_limit_group_decrement_write(ratelim_group, cfg_group_drain);
if (ratelim_group) {
Copy link
Member

Choose a reason for hiding this comment

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

It should never be called when ratelim_group is NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1 0x557e063d31e8 in group_drain_cb /home/runner/work/libevent/libevent/test/test-ratelim.c:271:2

I add this check to solve the null pointer dereference here. I don't understand why the code before my modifications didn't cause a crash...something wrong?

@azat
Copy link
Member

azat commented May 4, 2024

Now it fails

2024-04-30T13:26:43.6229111Z The following tests FAILED:
2024-04-30T13:26:43.6229473Z 	 81 - test-ratelim__group_lim (Timeout)
2024-04-30T13:26:43.6229627Z 	 82 - test-ratelim__con_lim (Timeout)
2024-04-30T13:26:43.6229809Z 	 83 - test-ratelim__group_con_lim (Timeout)
2024-04-30T13:26:43.6230003Z 	 84 - test-ratelim__group_con_lim_drain (Timeout)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@icy17 @azat