Covscan fixes #5498
Covscan fixes #5498
Conversation
Covscan is confused by dangling pointers in arrays after freeing. Its analyzer may decide to visit already visited list elements and since they weren't NULL-ed, it may consider double-free to happen in the code. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
covscan does consider 'ret' unitialized even though GET_ATTR/GET_ATTR_ARRAY macros have explicit and unconditional assignment to ret. This is confusing but causing actual failures in covscan runs. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
There are no checks that talloc()-ed symlink file name is not NULL. Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
| @@ -1207,6 +1209,9 @@ static int eval_response(pam_handle_t *pamh, size_t buflen, uint8_t *buf, | |||
| p += len; | |||
|
|
|||
| --c; | |||
|
|
|||
| free(env_item); | |||
There was a problem hiding this comment.
Probably I miss something, but man putenv:
int putenv(char *string);
The string pointed to by string becomes part of the environment, so altering the string changes the environment.
so we shouldn't free it in case we return 'PAM_SUCCESS'?
The question what should we do in case we return 'PAM_BUF_ERR' is more tricky.
| if (symlink_filename == NULL) { | ||
| return ENOMEM; | ||
| } | ||
|
|
There was a problem hiding this comment.
There are no checks that talloc()-ed symlink file name is not NULL.
Strictly speaking, there is a check:
sssd/src/sbus/server/sbus_server.c
Line 99 in 273ff9e
But having additional check here won't hurt. Though IMO it would be more suitable to use 'EINVAL' here.
|
Hi @abbra, thanks again for the patches. I had a couple of minor remarks/questions (please see inline) and just realized that probably you didn't receive a notification for inline comments... |
|
I did receive them but had no time to fix. If you have some time, please feel free to overtake this PR. I don't think I'll have much time until maybe mid March. |
|
Hi @abbra, 3 patches were pushed via #5526 This leaves I will close this PR. Please open a new one if you still want to propose [another version of] this patch. |
|
@alexey-tikhonov thanks for handling this. I am OK with dropping the remaining patch. |
No description provided.