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

Invalid read: Multi::close called twice #198

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Conversation

DBLouis
Copy link
Contributor

@DBLouis DBLouis commented Jun 10, 2020

Multi::close is called a second time in the Drop impl: https://docs.rs/curl/0.4.29/src/curl/multi.rs.html#728-732
Valgrind reports the following:

==5851== Invalid read of size 8
==5851==    at 0x560D4A5: curl_multi_cleanup (in /usr/lib/libcurl.so.4.6.0)
==5851==    by 0x4E75621: curl::multi::Multi::close (multi.rs:705)
==5851==    by 0x4E756AA: <curl::multi::Multi as core::ops::drop::Drop>::drop (multi.rs:730)
==5851==    by 0x4D86E76: core::ptr::drop_in_place (mod.rs:177)
==5851==    by 0x4D87D96: core::ptr::drop_in_place (mod.rs:177)
==5851==    by 0x4DD41B3: isahc::agent::AgentContext::run (agent.rs:495)

Removing the call to close seems to fix it, however some memory leaks remain.

@DBLouis DBLouis requested a review from sagebind as a code owner June 10, 2020 12:28
@DBLouis DBLouis changed the title Update agent.rs Invalid read: Multi::close called twice Jun 10, 2020
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #198 into master will decrease coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #198      +/-   ##
==========================================
- Coverage   69.85%   69.38%   -0.47%     
==========================================
  Files          33       33              
  Lines        1874     1852      -22     
==========================================
- Hits         1309     1285      -24     
- Misses        565      567       +2     
Impacted Files Coverage Δ
src/agent.rs 69.03% <ø> (-2.01%) ⬇️
src/metrics.rs 24.39% <0.00%> (-8.95%) ⬇️
src/handler.rs 74.37% <0.00%> (-1.14%) ⬇️
src/auth.rs 60.93% <0.00%> (-0.97%) ⬇️
src/text.rs 70.68% <0.00%> (ø)
src/config/dns.rs 0.00% <0.00%> (ø)
src/client.rs 62.34% <0.00%> (+0.51%) ⬆️
src/cookies/mod.rs 85.00% <0.00%> (+2.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a541bb...2decbf6. Read the comment docs.

@sagebind
Copy link
Owner

Hmm, yeah that's no good! This should probably be fixed upstream as well. Multi::close should probably have taken self, not &self. For now, its probably OK to merge this as-is until upstream is fixed, at which point we can put the manual call back.

The purpose of calling close manually was to ensure that any errors that that function returned would be visible instead of ignored.

Copy link
Owner

@sagebind sagebind left a comment

Choose a reason for hiding this comment

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

Thanks! 👏

@sagebind sagebind merged commit f4d2955 into sagebind:master Jun 10, 2020
@sagebind sagebind added this to the 0.9.4 milestone Jun 11, 2020
@DBLouis DBLouis deleted the patch-2 branch June 11, 2020 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants