`cluster_label_prop()` crashes R when using fixed labels

[ababaian]

`cluster_label_prop()` crashes R when using fixed labels

[ababaian]

I've narrowed down the issue, there are NA values being passed on to the intitial numeric vector of cluster_label_prop(). When the labels are not fixed this does not seem to be an issue, but if the NA integer are set as fixed, R crashes.

# R Crashes (as before)
lpa <- cluster_label_prop(g,
                          weights = E(g)$vrank,
                          initial = lab.df$int.label,
                          fixed   = lab.df$fixed)

# R does not crash
na.label <- is.na(lab.df$int.label)
lab.df$int.label[ na.label ] <- 99

lpa <- cluster_label_prop(g,
                          weights = E(g)$vrank,
                          initial = lab.df$int.label,
                          fixed   = lab.df$fixed)

# R Crashes
# lab.df$fixed[ 1 ] is TRUE
lab.df$int.label[ 1 ] <- NA

lpa <- cluster_label_prop(g,
                          weights = E(g)$vrank,
                          initial = lab.df$int.label,
                          fixed   = lab.df$fixed)

[szhorvat]

@Antonov548 Can you run this using ASAN and see if it's still present in the dev version (to become 2.0.4)? When not using ASAN, a lack of crash does not indicate that there is no bug (I can't repro it on my machine, but that doesn't mean no bug).

[ababaian]

Is there a good way to get you system log information from the time the crash happens which could help diagnose the problem?

[szhorvat]

I'm not sure, I don't think so.

It's good to note that passing NA values to igraph functions is almost never valid (certainly not here). The exceptions are storing attributes (NA values can be stored) and where a NA scalar has special meaning (e.g. weights=NA).

That said, there should not be a crash.

I believe the major issue here is that the R interface does not do any validation when converting to an integer vector (igraph_vector_int_t). See:

rigraph/src/rinterface_extra.c

Lines 3380 to 3388 in 128182d

	igraph_error_t R_SEXP_to_vector_int_copy(SEXP sv, igraph_vector_int_t *v) {
	igraph_integer_t n = Rf_xlength(sv);
	double *svv=REAL(sv);
	IGRAPH_CHECK(igraph_vector_int_init(v, n));
	for (igraph_integer_t i = 0; i<n; i++) {
	VECTOR(*v)[i] = (igraph_integer_t)svv[i];
	}
	return IGRAPH_SUCCESS;
	}

This is related to #1140, but for vectors rather than scalars. I noted it with a yellow mark in #840. Note that doing it for vectors may have a noticeable performance impact.

@krlmlr I would not make this issue block 2.0.4. A proper fix will be very time consuming.

[ababaian]

Agreed, the NA was actually an error on my side upstream of LPA, but having it take down the whole R session was annoying. Throwing an error if there are NA in int.label seems prudent, but if the error is not easily reproducible, then that may cause other errors in systems where it's working.

[szhorvat]

Yes, of course this should be fixed. The problem is that the proper fix is time-consuming, and requires a lot of care, as it involves reviewing some of the fundamental glue code between R and C, and not just this single function. This is why I recommended not blocking the next release on this issue.