On 3/9/18 4:50 AM, Langer, Christoph wrote:
Hi Chris,
Secondly, it doesn't accept the assert as length check and complains:
fixed_size_dest: You might overrun the 17-character fixed-size string this-
_name by copying name without checking the length.
Agreed that the assert is not a length check in product builds. However,
the only caller has a length check. Have you tried moving this length
check into set_name() and see if the problem goes away? Although I don't
suggest that as a fix. Just curious as to what the result would be.
When doing a length check in set_name(), coverity would be pleased. But still
we'd have to handle length violations by either guaranteeing or returning some
error return code, or quietly truncating. But you say you don't suggest it as
fix anyway...
BTW, I just realized I had been ignoring the set_arg() changes all this
time and focused on set_name(). So if any of the complaints are unique
to set_arg() please let me know.
No, nothing unique.
And, 3rd, it considers the risk as elevated:
parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
Is this a complaint about "name" being a source argument to strcpy(). If
so, I don't get this one. How are you going to copy "name" without
specifying it as an argument to something (strcpy, strncpy, memcpy,
etc). Besides, it is being passed to strcpy as a const argument. Makes
me wonder if adding const to the parameter declarations for both
set_name() and enqueue() would help.
I think coverity just considers this finding as elevated because the input data
isn't something static from inside the method but comes in as argument.
In my opinion the points are valid, because in opt builds there would be no
length check.
But there is a length check in the caller. Does coverity not see checks up the
call chain?
Obviously not.
I really think it would be easiest to go to my proposed patch. And it doesn't
come with much cost and the place probably isn't performance relevant.
I'm not worried about performance. To me it has more to do with taking
easily to read code and changing it into something that someone would
stare at for a bit before figuring out what it's doing, and then ask
"Why so complicated?". Coverity is suppose to help us make our code
better. I don't see that being the case here. If in the end your changes
are the simplest approach to quieting coverity, then I guess that's what
we should go with. However, I'm still not convinced we really fully why
converity is not happy with a strcpy that can be statically shown to be
safe. Is is a coverity bug? Is there a call path we are missing?
Something else that makes it hard for coverity to statically check this?
That's one reason I'd like to see what happens if a check is put
directly in set_name.
OK, so let me summarize:
The code as it is right now has a little issue - which isn't obvious at a quick
glance by the way.
It can be fixed like I suggested. This would add two lines of code at each
place and one can argue about how easy it is to understand. To me it seems as
understandable as it was before - but I'm probably a bit concerned here.
In terms of readability, I was referring back to the original code that
just had the strlen. It was the original coverity fix to that code that
introduced readability issue. You aren't really doing much to make it
less readable.
I can suggest an alternative which might be easier to read:
http://cr.openjdk.java.net/~clanger/webrevs/8199010.1/ It comes at the cost of
2 calls to strlen() in dbg builds but it has one line of code less and might be
more straightforward to understand.
All larger refactoring of set_name() and set_arg() is beyond the scope of my
change.
I like this version better, although it doesn't change my opinion that
this is still all jumping through hoops to get coverity to stop
complaining about something that is perfectly fine.
Now I'd really like if you could accept one of my 2 proposals, given that also
Thomas and David think it's ok. I want to get this done now. 😊 Maybe you can
even sponsor it...
Yeah, I'm ok with the change. I've said my peace and don't just want to
get in the way of a simple fix. Yes, I can also sponsor it for you.
cheers,
Chris
Thanks & Best regards
Christoph
