#3841 The simultaneous use of strncpy and a length-check in client code is confusing Coverity
Closed: Fixed 5 months ago by jhrozek. Opened 8 months ago by jhrozek.

This is a quite minor issue, but we should strive for our code to be easily understandable by Coverity.

In the common.c client-side module in sss_cli_open_socket, we first have this check:

542     if (sizeof(nssaddr.sun_path) <= strlen(socket_name) + 1) {
 543         *errnop = EINVAL;
 544         return -1;
 545     }

But then, we use strncpy and we tell it to write to sizeof(nsaddr.sun_path)

strncpy(nssaddr.sun_path, socket_name, sizeof(nssaddr.sun_path));

This has two issues:
1. strncpy always writes the requested number of bytes, so the rest of the buffer is needlesly zeroed out
2. scanners like Coverity think that, because the strncpy call doesn't itself leave room for NULL-termination, that the resulting buffer can be unterminated. This is not the case, because the check above guards us from using exactly the same sizes of input and the buffer, but it's better to avoid confusing Coverity

Metadata Update from @jhrozek:
- Issue tagged with: easyfix

8 months ago

Metadata Update from @jhrozek:
- Issue set to the milestone: SSSD Patches welcome

8 months ago

Metadata Update from @atikhonov:
- Issue assigned to atikhonov

5 months ago

Relevant Covscan quote:

sssd-2.0.99/src/sss_client/common.c:549: buffer_size_warning: Calling strncpy with a maximum size argument of 108 bytes on destination array "nssaddr.sun_path" of size 108 bytes might leave the destination string unterminated.
#  547|       memset(&nssaddr, 0, sizeof(struct sockaddr_un));
#  548|       nssaddr.sun_family = AF_UNIX;
#  549|->     strncpy(nssaddr.sun_path, socket_name, sizeof(nssaddr.sun_path));
#  550|   
#  551|       sd = socket(AF_UNIX, SOCK_STREAM, 0);

Metadata Update from @jhrozek:
- Issue close_status updated to: Fixed
- Issue status updated to: Closed (was: Open)

5 months ago

Metadata Update from @jhrozek:
- Issue set to the milestone: SSSD 2.1 (was: SSSD Patches welcome)

5 months ago

Login to comment on this ticket.