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

Add length bounds to string conversion function #208

Closed

Conversation

chrhansk
Copy link
Contributor

So far the conversions do not include length checks on the target string, causing memory faults when the Fortran strings are longer than their C counterparts. This patch attempts to circumvent the issue by trimming the output to match size restrictions.

@jfowkes
Copy link
Contributor

jfowkes commented Jun 17, 2024

Thanks @chrhansk. @mjacobse would you be able to review?

@jfowkes jfowkes requested a review from mjacobse June 17, 2024 12:17
@jfowkes jfowkes added the bug label Jun 17, 2024
@mjacobse
Copy link
Collaborator

I'm afraid I don't really understand what this change is about. What problem is it supposed to solve? As far as I can tell, this code only passes the length of the fstr argument to convert_string_f2c as maxlen. That seems redundant, since fstr already carries that information, LEN(fstr) == maxlen will always be true. The following new length calculation seems to come down to truncating the input fstr to 70 characters. So the longest possible output cstr would now be 70 characters + C_NULL_CHAR instead of before 72 characters + C_NULL_CHAR, as also noted in the rb_read.c example:

char title[73]; // Maximum length 72 character + '\0'

I am not sure in what way this would be an improvement, it just seems to truncate the input fstr unnecessarily. If the thought is to improve safety by doing a bounds check on the actual target C string, then spral_rb_peek, spral_rb_read and spral_rb_read_ptr32 would need new arguments that a user has to set to the size of the buffers that they provide. I am not sure how valuable that would really be, with the maximum size being known fixed small constants. But I could at least see some merit there.

Please let me know if I misunderstood anything.

@chrhansk
Copy link
Contributor Author

You are right, I read the Fortran documentation and did not notice the subtle difference in the definition of the arguments (the increase of one in the respective lengths). My solution is clearly invalid in this regard, sorry.

@chrhansk chrhansk closed this Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants