ralna / spral

Sparse Parallel Robust Algorithms Library
https://ralna.github.io/spral/
Other
105 stars 27 forks source link

Add length bounds to string conversion function #208

Closed chrhansk closed 3 months ago

chrhansk commented 3 months ago

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 commented 3 months ago

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

mjacobse commented 3 months ago

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: https://github.com/ralna/spral/blob/7fc5ec3ad16cc8d03578e1e539ffc37eef8ffe44/examples/C/rutherford_boeing/rb_read.c#L10

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 commented 3 months ago

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.