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

Prevent possible leaks #169

Merged
merged 2 commits into from Apr 1, 2015
Merged

Prevent possible leaks #169

merged 2 commits into from Apr 1, 2015

Conversation

@acv
Copy link
Contributor

@acv acv commented Mar 29, 2015

In some error scenarios, HTTP request objects could not have been freed.

Prompted by valgrind results from #34

acv added 2 commits Mar 29, 2015
@acv

This comment has been minimized.

Copy link
Owner Author

@acv acv commented on src/gateway.c in ae581b3 Mar 29, 2015

continue was not needed as no other branch from the if would be take until end of body of loop.

@acv
Copy link
Contributor Author

@acv acv commented Mar 29, 2015

3 out of 4 builds in the matrix failed because Travis couldn't git pull from github due to ongoing DDoS.

@acv
Copy link
Contributor Author

@acv acv commented Mar 29, 2015

All build passed now that Github is accessible.

@mhaas
Copy link
Contributor

@mhaas mhaas commented Mar 29, 2015

Thanks. I will test tomorrow with the load tester. Now that it's not crashing anymore, I'm seeing some nice memory leaks and (possibly) hanging threads. That is, before applying this patch.

@mhaas
Copy link
Contributor

@mhaas mhaas commented Mar 29, 2015

I'm running this patch and your refactor patch and the load tester is behaving a bit weird... it seems that one of the HTTP call raises an exception. When I print it, I only get the empty string ''. Of course, the request does not seem to fire. I will investigate further tomorrow.

@@ -443,7 +443,9 @@ main_loop(void)
* values that are not -1, 0 or 1. */
if (webserver->lastError == -1) {
/* Interrupted system call */
continue; /* restart loop */
if (NULL != r) {

This comment has been minimized.

@mhaas

mhaas Mar 31, 2015
Contributor

Shouldn't we have a DEBUG log here? Or is an interrupted system call something that happens regularly?

This comment has been minimized.

@acv

acv Mar 31, 2015
Author Contributor

It should happen a lot when a gateway is not in use.

This comment has been minimized.

@mhaas

mhaas Apr 1, 2015
Contributor

No, I don't think so. The timeout argument to select is NULL:

If timeout is NULL (no time‐out), select() can block indefinitely.

The load tester runs fine until, at some point, I am seeing a lot of bad (empty) HTTP status lines which possibly come from somewhere in that code path. I added a debug print and the bad status lines and webServr->lastError == -1 do coincide.

@mhaas
Copy link
Contributor

@mhaas mhaas commented Apr 1, 2015

Ah, I think the problem happens because server->lastError is never reset. This way, the code always takes the error handling path even if the request was actually succesfull.

mhaas added a commit that referenced this pull request Apr 1, 2015
Prevent possible leaks
@mhaas mhaas merged commit d2492f5 into wifidog:master Apr 1, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@acv acv deleted the acv:leak_httpd branch Apr 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.