RFR (XS): 8199010: attachListener.hpp: Fix potential null termination issue found by coverity scans
christoph.langer at sap.com
Mon Mar 5 21:28:06 UTC 2018
> Asserts imply something that is suppose to never happen, but that you
> want to check for in debug builds to help uncover bugs. Given this,
> either we have a bug (and someone can pass in a name that is too long),
> or coverity is complaining about something that can never happen, or the
> assert is invalid. So the potential fixes are:
> -Fix the problem up the call chain were the invalid string can be passed in.
> -Tell coverity to clam up because having the string be too long is not
> -Leave in your fix but remove the assert.
I believe coverity has a valid point here for the case that strlen(name) == name_length_max or strlen(arg) == arg_length_max. In that case, memcpy would copy exactly the bytes for the strings without the terminating zero. And as zero initialization of c++ members is not guaranteed (as far as I know), the name_length_max + 1 byte or arg_legth_max + 1 could theoretically have nonzero values.
Furthermore, I think in this case it makes sense to have an assertion because, as you state, in the debug builds you want to see any potential bug uncovered at the cost of a JVM exit. But in an opt build you want to be rather stable, even in case you get names and arguments passed that are too long. I don't want to go into the details of potential calling paths how that can happen, though... But even in case there are length violations in attach operation names or its arguments, the operations would most likely result in no success which is uncritical to a running VM.
So wouldn't you agree that my change is fine as is?
Submission-repo testing reported no errors.
More information about the hotspot-runtime-dev