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.

Sign up for GitHub

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

feat: Use safe accessor to vector elements #1633

Merged
merged 6 commits into from
Jan 2, 2025
Merged

feat: Use safe accessor to vector elements #1633

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ddf5b5a
use safety accessor
Antonov548 Dec 11, 2024
db13e74
Update src/rinterface_extra.c
Antonov548 Dec 18, 2024
0d4bd3d
use R_xlen_t
Antonov548 Dec 18, 2024
0c4b289
fix compilation errors
Antonov548 Dec 18, 2024
1a4841f
use out pointers
Antonov548 Jan 2, 2025
fc5c148
Merge branch 'main' into f-1631-vector-index
krlmlr Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/rinterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,7 @@ igraph_error_t R_SEXP_to_attr_comb(SEXP input, igraph_attribute_combination_t *c
void R_check_int_scalar(SEXP value);
void R_check_real_scalar(SEXP value);
void R_check_bool_scalar(SEXP value);

igraph_integer_t R_get_int_scalar(SEXP sexp, int index);
igraph_real_t R_get_real_scalar(SEXP sexp, int index);
igraph_bool_t R_get_bool_scalar(SEXP sexp, int index);
Antonov548 marked this conversation as resolved.
Show resolved Hide resolved
38 changes: 31 additions & 7 deletions src/rinterface_extra.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,30 @@ void R_check_bool_scalar(SEXP value)
}
}

igraph_integer_t R_get_int_scalar(SEXP sexp, int index)
Antonov548 marked this conversation as resolved.
Show resolved Hide resolved
{
if (Rf_length(sexp) < index + 1)
igraph_errorf("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
__FILE__, __LINE__, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

Suggested change
igraph_errorf("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
__FILE__, __LINE__, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
Rf_error("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
file, line, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));

with file and line passed from the invocation point (e.g., https://github.com/igraph/rigraph/pull/1633/files#diff-44d6b09dbc4b7f024f1b2df8dcc51a6e3d489408a6f82def8dd49ffb1bcd5dc4R4610) .

Copy link
Member

Choose a reason for hiding this comment

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

This won't properly unwind the "finally" stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still better than undefined behavior.

I don't have a good answer here. Let's draft and iterate.

Copy link
Contributor Author

@Antonov548 Antonov548 Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This won't properly unwind the "finally" stack.

Is it not only the way to properly unwind the finally stack is to call igraph_error() directly?

Copy link
Member

@szhorvat szhorvat Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the C core we use this approach:

igraph_error_t R_get_int_scalar(SEXP sexp, R_xlen_t index, igraph_integer_t *res) {
    if (Rf_xlength(sexp) <= index) {
        IGRAPH_ERRORF("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".", IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
    }
    *res = (igraph_integer_t)REAL(sexp)[index];
    return IGRAPH_SUCCESS;
}

Then R_get_int_scalar returns an error code as usual, and can be wrapped in IGRAPH_CHECK or IGRAPH_R_CHECK.

Would this work for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the C core we use this approach:

igraph_error_t R_get_int_scalar(SEXP sexp, R_xlen_t index, igraph_integer_t *res) {
    if (Rf_xlength(sexp) <= index) {
        IGRAPH_ERRORF("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".", IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
    }
    *res = (igraph_integer_t)REAL(sexp)[index];
    return IGRAPH_SUCCESS;
}

Then R_get_int_scalar returns an error code as usual, and can be wrapped in IGRAPH_CHECK or IGRAPH_R_CHECK.

Would this work for you?

Yes, this is what I was thinking about. Thanks.
I'll try this way.

Copy link
Member

Choose a reason for hiding this comment

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

Is it not only the way to properly unwind the finally stack is to call igraph_error() directly?

Yes in the sense that igraph_error() should be used, but normally it is called through IGRAPH_ERROR, which ensures returning from the function with an error code, which can then be processed by IGRAPH_CHECK.

In igraph 0.10, the unwinding process can potentially have multiple stages, each of which need to be triggered appropriately. This means that igraph_error() should not blindly longjmp. That might leave some stages untriggered.

This is why in the R interface we have IGRAPH_R_CHECK which must only be used at the top level. Only at this level is it safe to longjmp from the error handler, i.e. to call Rf_error(). IGRAPH_R_CHECK controls whether Rf_error() is called by igraph_error() by setting a global flag.

Now one might argue that R_get_int_scalar() is (or should be) only called at the top level anyway, so we can forget all about this. (Note that I am not actually sure whether this is true, there might be some non-top-level functions where things like REAL(sexp)[0] are still used!) If you decide to not worry about this, there's a risk that someone will use R_get_int_scalar() in an inappropriate context in the future, which causes a bug that's only triggered in an error condition, and will be very painful to track down. Thus if you go this route, I recommend clearly documenting what check function is allowed to be called in what context, and perhaps even naming these functions in such a way that any misuse is immediately apparent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thus if you go this route, I recommend clearly documenting what check function is allowed to be called in what context, and perhaps even naming these functions in such a way that any misuse is immediately apparent.

Not sure right now how to manage this, but I would think that functions which are have SEXP type as argument should be used only in top level functions. Which is also the case for R_get_int_scalar(). Maybe it make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that R_get_int_scalar() and friends should only be called at the top level and before any finalizers are set up. Also agree to document the purpose and the constraints.

return (igraph_integer_t)REAL(sexp)[index];
}

igraph_real_t R_get_real_scalar(SEXP sexp, int index)
{
if (Rf_length(sexp) < index + 1)
igraph_errorf("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
__FILE__, __LINE__, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
return (igraph_real_t)REAL(sexp)[index];
}

igraph_bool_t R_get_bool_scalar(SEXP sexp, int index)
{
if (Rf_length(sexp) < index + 1)
igraph_errorf("Wrong index. Attempt to get element with index %" PRIuPTR " from vector of length %" PRIuPTR ".",
__FILE__, __LINE__, IGRAPH_EINVAL, (uintptr_t) index, (uintptr_t) Rf_xlength(sexp));
return (igraph_bool_t)LOGICAL(sexp)[index];
}

SEXP R_igraph_i_lang7(SEXP s, SEXP t, SEXP u, SEXP v, SEXP w, SEXP x, SEXP y)
{
PROTECT(s);
Expand Down Expand Up @@ -4583,7 +4607,7 @@ SEXP R_igraph_get_shortest_paths(SEXP graph, SEXP pfrom, SEXP pto,
SEXP palgo) {

igraph_t g;
igraph_integer_t from=(igraph_integer_t) REAL(pfrom)[0];
igraph_integer_t from=R_get_int_scalar(pfrom, 0);
igraph_vs_t to;
igraph_vector_int_t to_data;
igraph_neimode_t mode=(igraph_neimode_t) Rf_asInteger(pmode);
Expand Down Expand Up @@ -4691,9 +4715,9 @@ SEXP R_igraph_get_shortest_paths(SEXP graph, SEXP pfrom, SEXP pto,
SEXP R_igraph_star(SEXP pn, SEXP pmode, SEXP pcenter) {

igraph_t g;
igraph_integer_t n=(igraph_integer_t) REAL(pn)[0];
igraph_integer_t mode=(igraph_integer_t) REAL(pmode)[0];
igraph_integer_t center=(igraph_integer_t) REAL(pcenter)[0];
igraph_integer_t n=R_get_int_scalar(pn, 0);
igraph_integer_t mode=R_get_int_scalar(pmode, 0);
igraph_integer_t center=R_get_int_scalar(pcenter, 0);
SEXP result;

IGRAPH_R_CHECK(igraph_star(&g, n, (igraph_star_mode_t) mode, center));
Expand Down Expand Up @@ -5935,9 +5959,9 @@ SEXP R_igraph_grg_game(SEXP pn, SEXP pradius, SEXP ptorus,

igraph_t g;
igraph_integer_t n=(igraph_integer_t) REAL(pn)[0];
igraph_real_t radius=REAL(pradius)[0];
igraph_bool_t torus=LOGICAL(ptorus)[0];
igraph_bool_t coords=LOGICAL(pcoords)[0];
igraph_real_t radius=R_get_real_scalar(pradius, 0);
igraph_bool_t torus=R_get_bool_scalar(ptorus, 0);
igraph_bool_t coords=R_get_bool_scalar(pcoords, 0);
igraph_vector_t x, y, *px=0, *py=0;
SEXP result;

Expand Down
Loading