clang-tidy: fix and/or silence issues found, and more #1561
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cc15b6b to
4aafbdf
Compare
|
Remaining 4 detected issues in Detailsin CI: |
6d859b6 to
3164745
Compare
|
I silenced the 4, valid-looking issues in Would there be better ways to fix them? |
transport.c issues
|
@vszakats I have a bit of time today, I will look at them and see if I can figure out where things would go off the rails. |
|
Warning: |
|
Warning: |
|
Warning: |
|
Warning: |
clang-tidy is coming up with a plausible trace, at least, but it can be wrong, or miss context. (in #1561 (comment) under Details) |
|
@willco007, @MichaelBuckley Thanks for looking into these! If I hear you right, all 4 fixes are fine, and 3 seem impossible combinations in reality. They also won't ruin anything. Therefore, I'm going for a merge after cleaning up, making a PR msg, etc. |
transport.c issues```
src/misc.c:375:18: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign]
375 | *datalen = (unsigned int)dlen;
| ^ ~~~~~~~~~~~~~~~~~~
```
```
src/publickey.c:53:16: warning: Excessive padding in 'struct _LIBSSH2_PUBLICKEY_CODE_LIST' (8 padding bytes, where 0 is optimal). Optimal fields order: name, code, name_len, consider reordering the fields or adding explicit padding members [clang-analyzer-optin.performance.Padding]
53 | typedef struct _LIBSSH2_PUBLICKEY_CODE_LIST
| ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
54 | {
| ~
55 | int code;
| ~~~~~~~~~
56 | const char *name;
| ~~~~~~~~~~~~~~~~~
57 | int name_len;
| ~~~~~~~~~~~~~
58 | } LIBSSH2_PUBLICKEY_CODE_LIST;
| ~
```
```
src/packet.c:883:25: warning: Null pointer passed as 1st argument to memory comparison function [clang-analyzer-unix.cstring.NullArg]
883 | memcmp(name, "server-sig-algs", 15) == 0) {
| ^ ~~~~
```
```
src/packet.c:894:29: warning: Null pointer passed as 2nd argument to memory copy function [clang-analyzer-unix.cstring.NullArg]
894 | memcpy(session->server_sign_algorithms,
| ^
```
```
src/openssl.c:1098:9: warning: Value stored to 'rc' is never read [clang-analyzer-deadcode.DeadStores]
1098 | rc = 0;
| ^ ~
src/openssl.c:1098:9: note: Value stored to 'rc' is never read
1098 | rc = 0;
| ^ ~
src/openssl.c:2279:9: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
2279 | ret = -1;
| ^ ~~
src/openssl.c:2279:9: note: Value stored to 'ret' is never read
2279 | ret = -1;
| ^ ~~
src/openssl.c:2293:9: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
2293 | ret = -1;
| ^ ~~
src/openssl.c:2293:9: note: Value stored to 'ret' is never read
2293 | ret = -1;
| ^ ~~
src/openssl.c:2316:13: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
2316 | ret = -1;
| ^ ~~
src/openssl.c:2316:13: note: Value stored to 'ret' is never read
2316 | ret = -1;
| ^ ~~
```
```
src/knownhost.c:182:43: warning: Null pointer passed as 1st argument to string length function [clang-analyzer-unix.cstring.NullArg]
182 | salt, strlen(salt));
| ^
```
```
src/sftp.c:957:9: warning: Value stored to 'rc' is never read [clang-analyzer-deadcode.DeadStores]
957 | rc = LIBSSH2_ERROR_BUFFER_TOO_SMALL;
| ^
```
```
example/ssh2_agent_forwarding.c:153:9: warning: Value stored to 'rc' is never read [clang-analyzer-deadcode.DeadStores]
153 | rc = 1;
| ^ ~
```
```
tests/test_read.c:48:57: warning: Null pointer passed as 1st argument to string length function [clang-analyzer-unix.cstring.NullArg]
48 | (unsigned int)strlen(username));
| ^ ~~~~~~~~
```
```
tests/session_fixture.c:487:57: warning: Null pointer passed as 1st argument to string length function [clang-analyzer-unix.cstring.NullArg]
487 | (unsigned int)strlen(username));
| ^ ~~~~~~~~
```
```
src/userauth.c:1776:13: warning: Value stored to 'rc' is never read [clang-analyzer-deadcode.DeadStores]
1776 | rc = LIBSSH2_ERROR_NONE;
| ^
```
```
src/kex.c:3484:13: warning: Value stored to 'p' is never read [clang-analyzer-deadcode.DeadStores]
3484 | p += lang_sc_len + 4;
| ^ ~~~~~~~~~~~~~~~
```
```
src/wincng.c:79:9: error: macro is not used [clang-diagnostic-unused-macros,-warnings-as-errors]
79 | #define PEM_ECDSA_HEADER "-----BEGIN OPENSSH PRIVATE KEY-----"
| ^
src/wincng.c:80:9: error: macro is not used [clang-diagnostic-unused-macros,-warnings-as-errors]
80 | #define PEM_ECDSA_FOOTER "-----END OPENSSH PRIVATE KEY-----"
| ^
```
```
src/wincng.c:3614:15: error: Array access (from variable 'bignum') results in a null pointer dereference [clang-analyzer-core.NullDereference,-warnings-as-errors]
3614 | bignum[0] &= (unsigned char)((1 << bits) - 1);
| ^
```
```
example/scp_write_nonblock.c:142:9: error: Opened stream never closed. Potential resource leak [clang-analyzer-unix.Stream,-warnings-as-errors]
142 | fprintf(stderr, "failed to create socket.\n");
| ^~~~~~~
```
rewind returns an error condition in errno. errno is problematic
and reduces portability. Use fseek to avoid errno.
```
tests/session_fixture.c:443:14: error: Value of 'errno' was not checked and may be overwritten by function 'calloc' [clang-analyzer-unix.Errno,-warnings-as-errors]
443 | buffer = calloc(1, (size_t)len + 1);
| ^
```
rewind returns an error condition in errno. errno is problematic
and reduces portability. Use fseek to avoid errno.
```
src/userauth.c:683:9: error: Value of 'errno' was not checked and may be overwritten by function 'fclose' [clang-analyzer-unix.Errno,-warnings-as-errors]
683 | fclose(fd);
| ^
```
```
example/scp_write_nonblock.c:142:9: error: Opened stream never closed. Potential resource leak [clang-analyzer-unix.Stream,-warnings-as-errors]
142 | fprintf(stderr, "failed to create socket.\n");
| ^~~~~~~
```
```
example/sftp_write_sliding.c:148:9: error: Opened stream never closed. Potential resource leak [clang-analyzer-unix.Stream,-warnings-as-errors]
148 | fprintf(stderr, "failed to create socket.\n");
| ^~~~~~~
```
``` ../../src/openssl.c:247:1: error: unused function '_libssh2_swap_bytes' [-Werror,-Wunused-function] _libssh2_swap_bytes(unsigned char *buf, unsigned long len) ^ ``` Detected now that openssl.c is no longer #included.
Avoid unnecessary inline assigments, that were ignored for the second block and replaceable with a `ret = 0` initialization for the first. Fix ignoring the failure of the second block, and calling mbedtls_rsa_check_privkey unconditionally.
Assessment by @willco007: Warning: `p->padding_length = first_block[0];` would require `blocksize` to be zero which can't happen. Assigning first_block to zero seems like a fine fix in this case. ``` src/transport.c:259:35: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign] 259 | p->padding_length = first_block[0]; | ^ ``` Full breakdown: ``` src/transport.c:259:35: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign] 259 | p->padding_length = first_block[0]; | ^ src/transport.c:400:8: note: Assuming the condition is false 400 | if(session->state & LIBSSH2_STATE_EXCHANGING_KEYS && | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:400:55: note: Left side of '&&' is false 400 | if(session->state & LIBSSH2_STATE_EXCHANGING_KEYS && | ^ src/transport.c:418:8: note: Assuming field 'readPack_state' is equal to libssh2_NB_state_jump1 418 | if(session->readPack_state == libssh2_NB_state_jump1) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:418:5: note: Taking true branch 418 | if(session->readPack_state == libssh2_NB_state_jump1) { | ^ src/transport.c:421:9: note: Control jumps to line 871 421 | goto libssh2_transport_read_point1; | ^ src/transport.c:871:18: note: Calling 'fullpacket' 871 | rc = fullpacket(session, encrypted); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:191:8: note: Assuming 'encrypted' is not equal to 0 191 | if(!encrypted || (!CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) && | ^~~~~~~~~~ src/transport.c:191:8: note: Left side of '||' is false src/transport.c:191:24: note: Assuming field 'crypt' is null 191 | if(!encrypted || (!CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) && | ^ src/libssh2_priv.h:1076:38: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:191:24: note: Left side of '&&' is false 191 | if(!encrypted || (!CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) && | ^ src/libssh2_priv.h:1076:62: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:191:23: note: Left side of '&&' is true 191 | if(!encrypted || (!CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) && | ^ src/transport.c:192:24: note: Field 'crypt' is null 192 | !CRYPT_FLAG_R(session, INTEGRATED_MAC))) { | ^ src/libssh2_priv.h:1076:56: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:192:24: note: Left side of '&&' is false 192 | !CRYPT_FLAG_R(session, INTEGRATED_MAC))) { | ^ src/libssh2_priv.h:1076:62: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:191:5: note: Taking true branch 191 | if(!encrypted || (!CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) && | ^ src/transport.c:196:8: note: Assuming field 'fullpacket_state' is equal to libssh2_NB_state_idle 196 | if(session->fullpacket_state == libssh2_NB_state_idle) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:196:5: note: Taking true branch 196 | if(session->fullpacket_state == libssh2_NB_state_idle) { | ^ src/transport.c:200:12: note: 'encrypted' is not equal to 0 200 | if(encrypted && remote_mac) { | ^~~~~~~~~ src/transport.c:200:12: note: Left side of '&&' is true src/transport.c:200:25: note: Assuming 'remote_mac' is non-null 200 | if(encrypted && remote_mac) { | ^~~~~~~~~~ src/transport.c:200:9: note: Taking true branch 200 | if(encrypted && remote_mac) { | ^ src/transport.c:205:16: note: Assuming 'etm' is not equal to 0 205 | if(etm) { | ^~~ src/transport.c:205:13: note: Taking true branch 205 | if(etm) { | ^ src/transport.c:228:16: note: Assuming the condition is false 228 | if(memcmp(macbuf, p->payload + p->total_num - mac_len, mac_len)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:228:13: note: Taking false branch 228 | if(memcmp(macbuf, p->payload + p->total_num - mac_len, mac_len)) { | ^ src/transport.c:234:21: note: 'etm' is not equal to 0 234 | else if(etm) { | ^~~ src/transport.c:234:18: note: Taking true branch 234 | else if(etm) { | ^ src/transport.c:244:22: note: Calling 'decrypt' 244 | rc = decrypt(session, p->payload + 4, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 245 | first_block, blocksize, FIRST_BLOCK); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:137:9: note: Assuming field 'crypt' is non-null 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^ src/libssh2_priv.h:1073:38: note: expanded from macro 'CRYPT_FLAG_L' 1073 | #define CRYPT_FLAG_L(session, flag) ((session)->local.crypt && \ | ^~~~~~~~~~~~~~~~~~~~~~ src/transport.c:137:9: note: Left side of '&&' is true 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^ src/libssh2_priv.h:1073:38: note: expanded from macro 'CRYPT_FLAG_L' 1073 | #define CRYPT_FLAG_L(session, flag) ((session)->local.crypt && \ | ^ src/transport.c:137:8: note: Assuming the condition is false 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:137:5: note: Taking false branch 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^ src/transport.c:140:11: note: Assuming 'len' is <= 0 140 | while(len > 0) { | ^~~~~~~ src/transport.c:140:5: note: Loop condition is false. Execution continues on line 174 140 | while(len > 0) { | ^ src/transport.c:174:5: note: Returning without writing to '*dest' 174 | return LIBSSH2_ERROR_NONE; /* all is fine */ | ^ src/transport.c:174:5: note: Returning zero, which participates in a condition later 174 | return LIBSSH2_ERROR_NONE; /* all is fine */ | ^~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:244:22: note: Returning from 'decrypt' 244 | rc = decrypt(session, p->payload + 4, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 245 | first_block, blocksize, FIRST_BLOCK); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:246:20: note: 'rc' is 0 246 | if(rc) { | ^~ src/transport.c:246:17: note: Taking false branch 246 | if(rc) { | ^ src/transport.c:253:20: note: Assuming 'decrypt_buffer' is non-null 253 | if(!decrypt_buffer) { | ^~~~~~~~~~~~~~~ src/transport.c:253:17: note: Taking false branch 253 | if(!decrypt_buffer) { | ^ src/transport.c:259:35: note: Assigned value is garbage or undefined 259 | p->padding_length = first_block[0]; | ^ ~~~~~~~~~~~~~~ ```
Assessment by @willco007: Warning: `p->padding_length = block[4];` would also require block size to be zero. ``` src/transport.c:619:39: warning: Assigned value is garbage or undefined [clang-analyzer-core.uninitialized.Assign] 619 | p->padding_length = block[4]; | ^ ~~~~~~~~ src/transport.c:400:8: note: Assuming the condition is false 400 | if(session->state & LIBSSH2_STATE_EXCHANGING_KEYS && | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:400:55: note: Left side of '&&' is false 400 | if(session->state & LIBSSH2_STATE_EXCHANGING_KEYS && | ^ src/transport.c:418:8: note: Assuming field 'readPack_state' is not equal to libssh2_NB_state_jump1 418 | if(session->readPack_state == libssh2_NB_state_jump1) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:418:5: note: Taking false branch 418 | if(session->readPack_state == libssh2_NB_state_jump1) { | ^ src/transport.c:426:12: note: Assuming the condition is false 426 | if(session->socket_state == LIBSSH2_SOCKET_DISCONNECTED) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:426:9: note: Taking false branch 426 | if(session->socket_state == LIBSSH2_SOCKET_DISCONNECTED) { | ^ src/transport.c:430:12: note: Assuming the condition is true 430 | if(session->state & LIBSSH2_STATE_NEWKEYS) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:430:9: note: Taking true branch 430 | if(session->state & LIBSSH2_STATE_NEWKEYS) { | ^ src/transport.c:439:12: note: 'encrypted' is 1 439 | if(encrypted) { | ^~~~~~~~~ src/transport.c:439:9: note: Taking true branch 439 | if(encrypted) { | ^ src/transport.c:440:16: note: Field 'crypt' is non-null 440 | if(CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/libssh2_priv.h:1076:56: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:440:16: note: Left side of '&&' is true 440 | if(CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/libssh2_priv.h:1076:38: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:440:16: note: Assuming the condition is false 440 | if(CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/libssh2_priv.h:1077:6: note: expanded from macro 'CRYPT_FLAG_R' 1077 | ((session)->remote.crypt->flags & LIBSSH2_CRYPT_FLAG_##flag)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:440:13: note: Taking false branch 440 | if(CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/transport.c:448:15: note: 'encrypted' is 1 448 | etm = encrypted && remote_mac ? remote_mac->etm : 0; | ^~~~~~~~~ src/transport.c:448:15: note: Left side of '&&' is true src/transport.c:448:28: note: Assuming 'remote_mac' is null 448 | etm = encrypted && remote_mac ? remote_mac->etm : 0; | ^~~~~~~~~~ src/transport.c:448:15: note: '?' condition is false 448 | etm = encrypted && remote_mac ? remote_mac->etm : 0; | ^ src/transport.c:461:16: note: Assuming 'remainbuf' is >= 0 461 | assert(remainbuf >= 0); | ^ /Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk/usr/include/assert.h:99:25: note: expanded from macro 'assert' 99 | (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __ASSERT_FILE_NAME, __LINE__, #e) : (void)0) | ^ src/transport.c:461:9: note: '?' condition is false 461 | assert(remainbuf >= 0); | ^ /Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk/usr/include/assert.h:99:6: note: expanded from macro 'assert' 99 | (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __ASSERT_FILE_NAME, __LINE__, #e) : (void)0) | ^ src/transport.c:463:12: note: Assuming 'remainbuf' is >= 'blocksize' 463 | if(remainbuf < blocksize || | ^~~~~~~~~~~~~~~~~~~~~ src/transport.c:463:12: note: Left side of '||' is false src/transport.c:464:13: note: Field 'crypt' is non-null 464 | (CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) | ^ src/libssh2_priv.h:1076:56: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:464:13: note: Left side of '&&' is true 464 | (CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET) | ^ src/libssh2_priv.h:1076:38: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:465:13: note: Left side of '&&' is false 465 | && ((ssize_t)p->total_num) > remainbuf)) { | ^ src/transport.c:517:12: note: Assuming field 'total_num' is 0 517 | if(!p->total_num) { | ^~~~~~~~~~~~~ src/transport.c:517:9: note: Taking true branch 517 | if(!p->total_num) { | ^ src/transport.c:524:37: note: 'etm' is 0 524 | ssize_t required_size = etm ? 4 : blocksize; | ^~~ src/transport.c:524:37: note: '?' condition is false src/transport.c:530:16: note: 'numbytes' is >= 'required_size' 530 | if(numbytes < required_size) { | ^~~~~~~~ src/transport.c:530:13: note: Taking false branch 530 | if(numbytes < required_size) { | ^ src/transport.c:540:16: note: 'etm' is 0 540 | if(etm) { | ^~~ src/transport.c:540:13: note: Taking false branch 540 | if(etm) { | ^ src/transport.c:545:21: note: 'encrypted' is 1 545 | else if(encrypted && session->remote.crypt->get_len) { | ^~~~~~~~~ src/transport.c:545:21: note: Left side of '&&' is true src/transport.c:545:34: note: Assuming field 'get_len' is null 545 | else if(encrypted && session->remote.crypt->get_len) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:545:18: note: Taking false branch 545 | else if(encrypted && session->remote.crypt->get_len) { | ^ src/transport.c:572:20: note: 'encrypted' is 1 572 | if(encrypted) { | ^~~~~~~~~ src/transport.c:572:17: note: Taking true branch 572 | if(encrypted) { | ^ src/transport.c:574:26: note: Calling 'decrypt' 574 | rc = decrypt(session, &p->buf[p->readidx], | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 575 | block, blocksize, FIRST_BLOCK); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:137:9: note: Assuming field 'crypt' is non-null 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^ src/libssh2_priv.h:1073:38: note: expanded from macro 'CRYPT_FLAG_L' 1073 | #define CRYPT_FLAG_L(session, flag) ((session)->local.crypt && \ | ^~~~~~~~~~~~~~~~~~~~~~ src/transport.c:137:9: note: Left side of '&&' is true 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^ src/libssh2_priv.h:1073:38: note: expanded from macro 'CRYPT_FLAG_L' 1073 | #define CRYPT_FLAG_L(session, flag) ((session)->local.crypt && \ | ^ src/transport.c:137:8: note: Assuming the condition is false 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:137:5: note: Taking false branch 137 | if(!CRYPT_FLAG_L(session, PKTLEN_AAD)) | ^ src/transport.c:140:11: note: Assuming 'len' is <= 0 140 | while(len > 0) { | ^~~~~~~ src/transport.c:140:5: note: Loop condition is false. Execution continues on line 174 140 | while(len > 0) { | ^ src/transport.c:574:26: note: Returning from 'decrypt' 574 | rc = decrypt(session, &p->buf[p->readidx], | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 575 | block, blocksize, FIRST_BLOCK); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:576:24: note: 'rc' is equal to LIBSSH2_ERROR_NONE 576 | if(rc != LIBSSH2_ERROR_NONE) { | ^~ src/transport.c:576:21: note: Taking false branch 576 | if(rc != LIBSSH2_ERROR_NONE) { | ^ src/transport.c:599:17: note: 'encrypted' is 1 599 | if(!encrypted || !CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^~~~~~~~~ src/transport.c:599:16: note: Left side of '||' is false 599 | if(!encrypted || !CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/transport.c:599:31: note: Field 'crypt' is non-null 599 | if(!encrypted || !CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/libssh2_priv.h:1076:56: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:599:31: note: Left side of '&&' is true 599 | if(!encrypted || !CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/libssh2_priv.h:1076:38: note: expanded from macro 'CRYPT_FLAG_R' 1076 | #define CRYPT_FLAG_R(session, flag) ((session)->remote.crypt && \ | ^ src/transport.c:599:13: note: Taking true branch 599 | if(!encrypted || !CRYPT_FLAG_R(session, REQUIRES_FULL_PACKET)) { | ^ src/transport.c:600:20: note: Assuming field 'packet_length' is >= 1 600 | if(p->packet_length < 1) { | ^~~~~~~~~~~~~~~~~~~~ src/transport.c:600:17: note: Taking false branch 600 | if(p->packet_length < 1) { | ^ src/transport.c:603:25: note: Assuming field 'packet_length' is <= LIBSSH2_PACKET_MAXPAYLOAD 603 | else if(p->packet_length > LIBSSH2_PACKET_MAXPAYLOAD) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ src/transport.c:603:22: note: Taking false branch 603 | else if(p->packet_length > LIBSSH2_PACKET_MAXPAYLOAD) { | ^ src/transport.c:607:20: note: 'etm' is 0 607 | if(etm) { | ^~~ src/transport.c:607:17: note: Taking false branch 607 | if(etm) { | ^ src/transport.c:619:39: note: Assigned value is garbage or undefined 619 | p->padding_length = block[4]; | ^ ~~~~~~~~ ```
Assessment by @willco007: Warning: `(encrypted ? remote_mac->mac_len : 0);` assumes `remote.crypt` goes from non-null to null...magically. Code reviewed with @MichaelBuckley and we couldn't see how this would be possible. Adding additional null check is a fine fix. ``` src/transport.c:627:34: error: Access to field 'mac_len' results in a dereference of a null pointer (loaded from variable 'remote_mac') [clang-analyzer-core.NullDereference,-warnings-as-errors] 627 | (encrypted ? remote_mac->mac_len : 0); | ^~~~~~~~~~ ```
Assessment by @willco007: Warning: `local_mac->mac_len` could potentially be hit if a crypt method was defined incorrectly. However, none of the crypt methods can hit this in reality. NULL check here is fine, it will prevent the crash and just fail to encrypt the value. ``` src/transport.c:1241:31: error: Access to field 'mac_len' results in a dereference of a null pointer (loaded from variable 'local_mac') [clang-analyzer-core.NullDereference,-warnings-as-errors] 1241 | int authlen = local_mac->mac_len; | ^~~~~~~~~ ```
vszakats
added a commit
that referenced
this pull request
Mar 24, 2025
Instead build all crypto backend sources always, and exclude inactive ones with guards. To play better with code checkers and compilers that are blind to included C sources, e.g. clang with certain compiler warnings and clang-tidy. We continue to include `blowfish.c`. Follow-up to 4f0f4bf #941 Cherry-picked from #1561
vszakats
added a commit
that referenced
this pull request
Mar 24, 2025
To prepare for the addition of a clang-tidy job for Windows. Cherry-picked from #1561
5ca35cd to
1873543
Compare
1 task
1873543 to
d0b6447
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
the second block and replaceable with a
ret = 0initialization forthe first one.
mbedtls_rsa_check_privkey()unconditionally._libssh2_base64_decode().memcpysrc.rcerror assigment with_libssh2_error()call.rewind()withfseek().rewind()returns an error condition inerrno.errnoisproblematic and reduces portability. Use
fseek()to avoid it.sign_frommemory(). Possible false positive.rcshould be setupstream if the callback is NULL.
sign_fromfile(). clang-tidy did not warn about this one, butlet's match
sign_frommemory()anyway.tests:
usernamenot set, to avoidstrlen(NULL).rewind()withfseek().rewind()returns an error condition inerrno.errnoisproblematic and reduces portability. Use
fseek()to avoid it.usernamenot set, to avoidstrlen(NULL).examples:
Details in the subcommits under the PR.
Thanks-to: Michael Buckley
Thanks-to: Will Cosgrove