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 regress_ws.c #1621

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

Fix potential Null pointer dereference in regress_ws.c #1621

wants to merge 3 commits into from

Conversation

icy17
Copy link
Contributor

@icy17 icy17 commented Apr 22, 2024

Fix #1604

@icy17
Copy link
Contributor Author

icy17 commented Apr 22, 2024

I'm not sure if I should close fd before tt_assert...

@azat
Copy link
Member

azat commented Apr 29, 2024

I'm not sure if I should close fd before tt_assert...

Yes, it should be done under end label, and you need to initialize fd with -1

azat
azat requested changes 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.

.

azat
azat reviewed Apr 29, 2024
View reviewed changes
test/regress_ws.c Outdated
@@ -388,4 +390,5 @@ http_ws_test(void *arg)
end:
if (bev)
bufferevent_free(bev);
EVUTIL_CLOSESOCKET(fd);
Copy link
Member

Choose a reason for hiding this comment

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

if (fd != EVUTIL_INVALID_SOCKET) (and use it for initilzation), and also let's add a check for http

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the http is not used here? I'm not quite sure where I'm supposed to add the check.
Additionally, I should initialize fd to EVUTIL_INVALID_SOCKET and check if it is EVUTIL_INVALID_SOCKET before closing, is that right?

Copy link
Member

Choose a reason for hiding this comment

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

I noticed that the http is not used here?

It is used it creates http server, see usage of port.

I'm not quite sure where I'm supposed to add the check.

The http initialization may fail, so - if (!http) goto err

Additionally, I should initialize fd to EVUTIL_INVALID_SOCKET and check if it is EVUTIL_INVALID_SOCKET before closing, is that right?

Yes

@icy17 icy17 force-pushed the add_null_check_regress_ws branch from 76be7f0 to a2eff65 Compare April 29, 2024 07:05
azat
azat requested changes May 4, 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. 

/home/runner/work/libevent/libevent/test/regress_ws.c:351:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  351 |  struct evbuffer *out;
      |  ^~~~~~

Coeur
Coeur reviewed May 6, 2024
View reviewed changes
test/regress_ws.c Outdated Show resolved Hide resolved
@azat @Coeur
Update test/regress_ws.c
1e053b7 
Co-authored-by: Cœur <coeur@gmx.fr>
@Coeur
Copy link
Contributor

Coeur commented May 23, 2024 •
edited

There seems to be a lot of failures with:

[warn] evbuffer_file_segment_materialize: mmap(5, 0, 0) failed: Bad file descriptor

Could those failures be related to those changes (more specifically, the if (!http) goto end;)?

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

@Coeur Coeur Coeur left review comments

@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.

Potential Null pointer dereference in regress_ws.c
3 participants
@icy17 @azat @Coeur