fix: fixed memory leak in sample_degseq()

[szhorvat]

CC @Antonov548

[aviator-app]

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

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.

[szhorvat]

@Antonov548 I'm looking at rinterface_extra.c and I see that several updates were made without regard to memory management. Can you please have a look at some other functions that were updated and make the necessary corrections?

Here's one example:

rigraph/src/rinterface_extra.c

Lines 7335 to 7357 in 5e1c7b4

	SEXP R_igraph_community_to_membership2(SEXP pmerges, SEXP pvcount,
	SEXP psteps) {
	igraph_matrix_int_t merges;
	igraph_integer_t vcount=(igraph_integer_t) REAL(pvcount)[0];
	igraph_integer_t steps=(igraph_integer_t) REAL(psteps)[0];
	igraph_vector_int_t membership;
	SEXP result;
	R_SEXP_to_matrix_int(pmerges, &merges);
	igraph_vector_int_init(&membership, 0);
	IGRAPH_FINALLY(igraph_vector_int_destroy, &membership);
	IGRAPH_R_CHECK(igraph_community_to_membership(&merges, vcount, steps, &membership, 0));
	PROTECT(result=R_igraph_vector_int_to_SEXP(&membership));
	igraph_matrix_int_destroy(&merges);
	igraph_vector_int_destroy(&membership);
	IGRAPH_FINALLY_CLEAN(1);
	UNPROTECT(1);
	return result;
	}

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_FINALLY_CLEAN than the preceding number of destroy calls, then something's wrong:

  igraph_matrix_int_destroy(&merges);
  igraph_vector_int_destroy(&membership);
  IGRAPH_FINALLY_CLEAN(1);

Additionally, the IGRAPH_R_CHECK are missing from the R_SEXP_to_matrix_int and igraph_vector_int_init

I suggest starting by searching the file for IGRAPH_FINALLY_CLEAN and making corrections wherever the number doesn't match the number of destroy calls. This won't catch all issues, but it should catch many.

We need to be careful with changes where memory management is involved because finding issues later is difficult and painful.

[aviator-app]

This pull request failed to merge: some CI status(es) failed. Once the issues are resolved, remove the blocked label and re-queue the pull request. Note that the pull request will be automatically re-queued if it has the mergequeue label.

Failed CI(s): Check macOS-latest (release)

[szhorvat]

P.S. Most such fixes are easier by moving to auto-generation. Here I'm blocked in doing that by #876

[Antonov548]

@Antonov548 I'm looking at rinterface_extra.c and I see that several updates were made without regard to memory management. Can you please have a look at some other functions that were updated and make the necessary corrections?

Here's one example:

rigraph/src/rinterface_extra.c

Lines 7335 to 7357 in 5e1c7b4

	SEXP R_igraph_community_to_membership2(SEXP pmerges, SEXP pvcount,
	SEXP psteps) {
	igraph_matrix_int_t merges;
	igraph_integer_t vcount=(igraph_integer_t) REAL(pvcount)[0];
	igraph_integer_t steps=(igraph_integer_t) REAL(psteps)[0];
	igraph_vector_int_t membership;
	SEXP result;
	R_SEXP_to_matrix_int(pmerges, &merges);
	igraph_vector_int_init(&membership, 0);
	IGRAPH_FINALLY(igraph_vector_int_destroy, &membership);
	IGRAPH_R_CHECK(igraph_community_to_membership(&merges, vcount, steps, &membership, 0));
	PROTECT(result=R_igraph_vector_int_to_SEXP(&membership));
	igraph_matrix_int_destroy(&merges);
	igraph_vector_int_destroy(&membership);
	IGRAPH_FINALLY_CLEAN(1);
	UNPROTECT(1);
	return result;
	}

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_FINALLY_CLEAN than the preceding number of destroy calls, then something's wrong:

  igraph_matrix_int_destroy(&merges);
  igraph_vector_int_destroy(&membership);
  IGRAPH_FINALLY_CLEAN(1);

Additionally, the IGRAPH_R_CHECK are missing from the R_SEXP_to_matrix_int and igraph_vector_int_init

I suggest starting by searching the file for IGRAPH_FINALLY_CLEAN and making corrections wherever the number doesn't match the number of destroy calls. This won't catch all issues, but it should catch many.

We need to be careful with changes where memory management is involved because finding issues later is difficult and painful.

Thanks for finding it. Seems sanitizer is good right now.
I'm agree that it would be much easier to fix with autogeneration.

Sometime it's really hard to find them all in 9k+ lines C file.

[Antonov548]

I guess we can merge it regardless devel and mac-os build.

[szhorvat]

Yes, it should be merged, but I'll leave out-of-order merges to @krlmlr :-)

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

[krlmlr]

s/AppVeyor/Aviator/

[szhorvat]

@Antonov548 Would you be able to deal with the second memory leak I point out in the comments above?

[krlmlr]

This PR still has failing checks with macOS and Ubuntu 20.04. What can we do about that?

[Antonov548]

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?

[krlmlr]

Maybe, let's see.

[krlmlr]

Thanks! This might be included in the main branch already.