Fix issues reported by Coverity - v1 #5260
Conversation
This is meant to provide a single path to the error case. This might help make things more clear for static checkers.
| de->username = de->password = de->will = MQTT_DONT_CARE; | ||
| de->will_retain = de->clean_session = MQTT_DONT_CARE; | ||
|
|
||
| char copy[strlen(rawstr)+1]; | ||
| strlcpy(copy, rawstr, sizeof(copy)); | ||
| copy = SCStrdup(rawstr); |
There was a problem hiding this comment.
I don't really understand the goal here. Why is the stack copy not working?
There was a problem hiding this comment.
It is working. I wanted to move the code path to use goto error everywhere to make sure that Coverity sees the complete lifetime of de. However, the compiler won't allow us to goto into a scope with a variably sized declaration (copy). So I chose to manage the memory manually here.
| int ret = 0; | ||
| int ov[MAX_SUBSTRINGS]; | ||
|
|
||
| ret = DetectParsePcreExec(&parse_regex, rawstr, 0, 0, ov, MAX_SUBSTRINGS); | ||
| if (ret < 1) { | ||
| SCLogError(SC_ERR_PCRE_MATCH, "invalid flag definition: %s", rawstr); | ||
| return NULL; | ||
| goto error; |
There was a problem hiding this comment.
I feel this use of the error label is adding unnecessary complexity. Its not going to do anything there, but now we need to follow this jump logic to realize this.
There was a problem hiding this comment.
This is just to address Coverity's complaint about us checking de for NULL in the previous code -- it thinks that it might be NULL but it has been dereferenced before. Apparently it didn't catch that in that case we already returned from the function before in that case.
There was a problem hiding this comment.
I think it tells you there is an issue: by checking it can be NULL, it assumes the programmer "knows" that it can be. But in all the paths leading to the NULL check its already dereferenced. So it alerts you that you may have a bug before the null check. Of course it were smarter it would see that de was already checked for null.
There was a problem hiding this comment.
Got it. So you agree we should not try to work around that and remove the check in these few cases?
| } | ||
|
|
||
| de = SCCalloc(1, sizeof(DetectMQTTConnectFlagsData)); | ||
| if (unlikely(de == NULL)) | ||
| return NULL; | ||
| goto error; |
There was a problem hiding this comment.
returning NULL here was fine. I think the issue coverity flagged is that after this we don't have to check de != NULL anymore
There was a problem hiding this comment.
True. That check was requested in a previous review though -- I would be happy to remove it in these 3 cases as I agree it it only complicates things. OK?
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/3855
Describe changes:
u8daandu32da. This is basically just to please static checkers, values set at the beginning will always be overwritten or not used afterwards.SCStrdup()instead of using standard library functions.Please note that I currently do not have access to the Coverity service, so this was a best effort to address the issue the checker is raising.