-
-
Notifications
You must be signed in to change notification settings - Fork 199
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
fix: fixed memory leak in sample_degseq()
#1356
Conversation
Current Aviator status
This PR was closed without merging. If you still want to merge this PR, re-open it.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
5967b8e
to
bf07505
Compare
@Antonov548 I'm looking at Here's one example: rigraph/src/rinterface_extra.c Lines 7335 to 7357 in 5e1c7b4
Notice that two new data structures are allocated, but only one is added to the finally stack. Generlaly, when you see a different number in igraph_matrix_int_destroy(&merges);
igraph_vector_int_destroy(&membership);
IGRAPH_FINALLY_CLEAN(1); Additionally, the I suggest starting by searching the file for We need to be careful with changes where memory management is involved because finding issues later is difficult and painful. |
This pull request failed to merge: some CI status(es) failed. Once the issues are resolved, remove the Failed CI(s): Check macOS-latest (release) |
P.S. Most such fixes are easier by moving to auto-generation. Here I'm blocked in doing that by #876 |
Thanks for finding it. Seems sanitizer is good right now. Sometime it's really hard to find them all in 9k+ lines C file. |
I guess we can merge it regardless |
Yes, it should be merged, but I'll leave out-of-order merges to @krlmlr :-) |
Thanks. @Antonov548: Can you please tweak our CI/CD to ignore the failing builds, in a separate PR? I'd very much like to keep using AppVeyor, but don't have the time to properly fix CI/CD. |
s/AppVeyor/Aviator/ |
@Antonov548 Would you be able to deal with the second memory leak I point out in the comments above? |
This PR still has failing checks with macOS and Ubuntu 20.04. What can we do about that? |
Merge this one when it will be green: #1357. Will it works? |
Maybe, let's see. |
Thanks! This might be included in the main branch already. |
bf07505
to
e7ea747
Compare
CC @Antonov548