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.

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

A null pointer may cause a segmentation fault. #865

Closed
dota17 opened this issue Aug 6, 2019 · 4 comments
Closed

A null pointer may cause a segmentation fault. #865

dota17 opened this issue Aug 6, 2019 · 4 comments

Comments

@dota17
Copy link
Contributor

dota17 commented Aug 6, 2019

The main function of the sample/signal-test.c file is defined as follows:

int main(int argc, char **argv)
{
	struct event *signal_int;
	struct event_base* base;
#ifdef _WIN32
	WORD wVersionRequested;
	WSADATA wsaData;

	wVersionRequested = MAKEWORD(2, 2);

	(void) WSAStartup(wVersionRequested, &wsaData);
#endif

	/* Initalize the event library */
	base = event_base_new();

	/* Initalize one event */
	signal_int = evsignal_new(base, SIGINT, signal_cb, event_self_cbarg());

	event_add(signal_int, NULL);

	event_base_dispatch(base);
	event_free(signal_int);
	event_base_free(base);

	return (0);
}

It is possible to have a null pointer returned in the event_new()。

#define evsignal_new(b, x, cb, arg)				\
	event_new((b), (x), EV_SIGNAL|EV_PERSIST, (cb), (arg))

struct event *
event_new(struct event_base *base, evutil_socket_t fd, short events, void (*cb)(evutil_socket_t, short, void *), void *arg)
{
	struct event *ev;
	ev = mm_malloc(sizeof(struct event));
	if (ev == NULL)
		return (NULL);
	if (event_assign(ev, base, fd, events, cb, arg) < 0) {
		mm_free(ev);
		return (NULL);
	}

	return (ev);
}

Once a null pointer is passed in, event_add() does't have a null protection, it will cause a segmentation fault.

int event_add(struct event *ev, const struct timeval *tv)
{
	int res;

	if (EVUTIL_FAILURE_CHECK(!ev->ev_base)) {
		event_warnx("%s: event has no event_base set.", __func__);
		return -1;
	}

	EVBASE_ACQUIRE_LOCK(ev->ev_base, th_base_lock);

	res = event_add_nolock_(ev, tv, 0);

	EVBASE_RELEASE_LOCK(ev->ev_base, th_base_lock);

	return (res);
}

Therefore, I came up with two ways to improve it.:

  1. Add an null protection in the event_add().
  2. Check the return value of event_new() before it is passed to event_add().

The above is just my suggestion, do you have any other else?

@ploxiln
Copy link
Contributor

ploxiln commented Aug 6, 2019

It's a test. If there is a null pointer segmentation fault, the test fails. If the allocation is checked for failure, and failure is detected, the test fails ... more cleanly. It's probably not worth the more verbose code for this extremely unlikely case which will be caught anyway.

@azat
Copy link
Member

azat commented Aug 8, 2019

Agree with @ploxiln , although I'm not against adding NULL check after evsignal_new().
@dota17 could you send a patch, or should I take care of this by myself?

@dota17
Copy link
Contributor Author

dota17 commented Aug 8, 2019

Yes, I am glad to submit a patch. Besides, I agree with @ploxiln too. We should terminate the test with an error message if the allocation is checked for failure. But we can't guarantee that everyone will do this when calling event_add(). So, I prefer to add NULL check. :)

@azat
Copy link
Member

azat commented Aug 8, 2019

But we can't guarantee that everyone will do this when calling event_add()

We shouldn't, this is not our job, event_add() never has NULL check for event
So I prefer to add NULL check into signal-test.c only, so simply if (!event) goto fail;

@azat azat closed this as completed in 101fbe3 Aug 9, 2019
azat pushed a commit to azat/libevent that referenced this issue Jun 28, 2020
Fixes: libevent#865
(cherry picked from commit 101fbe3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants