Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gssapi: default pam_gssapi_services to NULL in domain section and coverity fixes #5453

Closed
wants to merge 2 commits into from

Conversation

pbrezina
Copy link
Member

We need to distinguish when the option is not set in domain section and when
it is is explicitly disabled. Now if it is not set, domain->gssapi_services
is NULL and we'll use value from the pam section.

Without this change, the value in the pam section is ignored.

We need to distinguish when the option is not set in domain section and when
it is is explicitly disabled. Now if it is not set, domain->gssapi_services
is NULL and we'll use value from the pam section.

Without this change, the value in the pam section is ignored.
@alexey-tikhonov
Copy link
Member

Is this addon to #5367?

@pbrezina
Copy link
Member Author

Yes.

@pbrezina pbrezina changed the title gssapi: default pam_gssapi_services to NULL in domain section gssapi: default pam_gssapi_services to NULL in domain section and coverity fixes Jan 12, 2021
@pbrezina
Copy link
Member Author

I also included fix for coverity warnings.

@alexey-tikhonov
Copy link
Member

Hi,

there is a compilation warning:

../src/sss_client/pam_sss_gss.c:339:5: warning: ‘reply’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  339 |     free(reply);
      |     ^~~~~~~~~~~
../src/sss_client/pam_sss_gss.c:328:14: note: ‘reply’ was declared here
  328 |     uint8_t *reply;
      |              ^~~~~
../src/sss_client/pam_sss_gss.c:270:11: warning: ‘reply_len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  270 |     upn = malloc(reply_len * sizeof(char));
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/sss_client/pam_sss_gss.c:327:12: note: ‘reply_len’ was declared here
  327 |     size_t reply_len;
      |            ^~~~~~~~~

That's false positive but I think still better to suppress.

Otherwise ACK for the 2nd patch ("fix coverity issues").

@pbrezina
Copy link
Member Author

I do not see this warning, but a fix is squashed to the second patch.

@sumit-bose
Copy link
Contributor

Hi,

thanks for the patches, Coverity does not complain about the resource leaks anymore and my tests for the pam_gssapi_services config option went well. I just added an in-line comment.

bye,
Sumit

```
1. Defect type: RESOURCE_LEAK
7. sssd-2.4.0/src/sss_client/pam_sss_gss.c:556: leaked_storage: Variable "username" going out of scope leaks the storage it points to.
Expand
2. Defect type: RESOURCE_LEAK
3. sssd-2.4.0/src/sss_client/pam_sss_gss.c:321: leaked_storage: Variable "reply" going out of scope leaks the storage it points to.
Expand
3. Defect type: RESOURCE_LEAK
7. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260: leaked_storage: Variable "username" going out of scope leaks the storage it points to.
Expand
4. Defect type: RESOURCE_LEAK
6. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260: leaked_storage: Variable "upn" going out of scope leaks the storage it points to.
Expand
5. Defect type: RESOURCE_LEAK
7. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260: leaked_storage: Variable "target" going out of scope leaks the storage it points to.
Expand
6. Defect type: RESOURCE_LEAK
7. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260: leaked_storage: Variable "domain" going out of scope leaks the storage it points to.

1. Defect type: CLANG_WARNING
1. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260:16: warning[unix.Malloc]: Potential leak of memory pointed to by 'username'
Expand
2. Defect type: CLANG_WARNING
1. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260:16: warning[unix.Malloc]: Potential leak of memory pointed to by 'upn'
Expand
3. Defect type: CLANG_WARNING
1. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260:16: warning[unix.Malloc]: Potential leak of memory pointed to by 'target'
Expand
4. Defect type: CLANG_WARNING
1. sssd-2.4.0/src/sss_client/pam_sss_gss.c:260:16: warning[unix.Malloc]: Potential leak of memory pointed to by 'domain'
```

Also fix compilation warning
```
../src/sss_client/pam_sss_gss.c:339:5: warning: ‘reply’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  339 |     free(reply);
      |     ^~~~~~~~~~~
../src/sss_client/pam_sss_gss.c:328:14: note: ‘reply’ was declared here
  328 |     uint8_t *reply;
      |              ^~~~~
../src/sss_client/pam_sss_gss.c:270:11: warning: ‘reply_len’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  270 |     upn = malloc(reply_len * sizeof(char));
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/sss_client/pam_sss_gss.c:327:12: note: ‘reply_len’ was declared here
  327 |     size_t reply_len;
      |            ^~~~~~~~~
```
@pbrezina
Copy link
Member Author

Thanks, I added NULL initialization.

@sumit-bose
Copy link
Contributor

Hi,

thanks, ACK.

bye,
Sumit

@pbrezina
Copy link
Member Author

Pushed PR: #5453

  • master
    • 111b8b4 - pam_sss_gssapi: fix coverity issues
    • cc17362 - gssapi: default pam_gssapi_services to NULL in domain section

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Jan 15, 2021
@pbrezina pbrezina closed this Jan 15, 2021
@pbrezina pbrezina deleted the gssapi branch April 13, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants