Overall this is a solid first TLS implementation. The architecture is clean and the integration with existing transport abstractions is well thought out. Below are the issues I found, organized by severity.
CRITICAL / Bugs
1. Memory leaks: SSL and SSL_CTX not freed on client disconnect/delete transport_tcp.c:129-141
client_disconnect() only closes the fd and waits for SSH. client_delete() only frees the struct. Neither frees client->ssl or client->ssl_ctx. Every TLS client connection leaks an SSL object and an SSL_CTX.
When the server calls config_receive(), it does a raw malloc and only populates specific fields. use_tls, tls_cert, and tls_key are never initialized, leaving them as garbage. While nothing currently reads them on the server, this is technically undefined behavior.
4. tls_global_cleanup() is never called anywhere transport_tls.c:24-30
Declared in the header but no code path invokes it. OpenSSL resources (error strings, algorithms) are leaked on exit. The global g_ssl_ctx is also never assigned to, making the cleanup dead code.
5. Deprecated OpenSSL 1.0.x API transport_tls.c:17-22,29
SSL_library_init(), OpenSSL_add_all_algorithms(), SSL_load_error_strings(), EVP_cleanup() are all deprecated since OpenSSL 1.1.0 and removed in some OpenSSL 3.x builds. Since 1.1.0, initialization is automatic. These calls should be removed or guarded with #if OPENSSL_VERSION_NUMBER < 0x10100000L.
Security Concerns
6. No minimum TLS version enforced transport_tls.c:33-34
create_ssl_ctx doesn't call SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION). On older systems, TLS 1.0 or 1.1 could be negotiated — both are deprecated and have known vulnerabilities.
7. No server certificate verification on the client transport_tls.c:32-61
create_ssl_ctx for the client never calls SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...). The client connects to the server without verifying its certificate, making it vulnerable to MITM attacks. This significantly undermines the value of TLS.
8. SSL errors dumped to stderr transport_tls.c:38,46,52,58,79
ERR_print_errors_fp(stderr) leaks internal SSL error details (potentially including certificate paths, cipher info) to stdout. Should use the project's log_message system, or at minimum only log at debug level.
Design Issues
9. tls_global_init() called multiple times without guard client_send.c:62,216
Called in both send_files() and send_chunks_multithreaded(). While the underlying calls are idempotent, this is wasteful and should be called once at startup.
create_ssl_ctx has an if (cert && key) guard that silently creates a context without a client certificate. On the server side, --tls requires --cert and --key. This asymmetry is confusing — the user might think --tls --cert cert.pem works, but it silently skips loading the cert if --key isn't provided.
11. server_listen_tls duplicates server_listen transport_tls.c:93-134 vs transport_tcp.c:59-89
Nearly identical code (accept loop, fork, close fd). The only difference is the SSL handshake wrapper. Should share a common accept/fork loop or use a function pointer pattern.
12. -p port not validated server.c:177
atoi(argv[++i]) returns 0 on failure with no error checking. No range validation (1-65535).
Minor / Style
13. No server --help / usage message
The server accepts --stdio, --verbose, --tls, --cert, --key, -p but has no help output.
14. __thread inconsistency protocol.c:9-11 — io_read_fd and io_write_fd are __thread, but io_ssl is plain static. This works because of the fork-per-client model (each child has its own copy), but it's inconsistent and fragile if the threading model changes.
Summary
The most important fixes before merging:
Free SSL/SSL_CTX in client_disconnect/client_delete/server_delete (memory leaks)
Initialize TLS fields in config_receive (UB)
Add minimum TLS 1.2 version enforcement (security)
Add SSL_VERIFY_PEER on client + callback or flag (security)
Remove or gate deprecated OpenSSL API calls (portability)
## Code Review: TLS transport
Overall this is a solid first TLS implementation. The architecture is clean and the integration with existing transport abstractions is well thought out. Below are the issues I found, organized by severity.
---
### CRITICAL / Bugs
**1. Memory leaks: `SSL` and `SSL_CTX` not freed on client disconnect/delete**
`transport_tcp.c:129-141`
`client_disconnect()` only closes the fd and waits for SSH. `client_delete()` only frees the struct. Neither frees `client->ssl` or `client->ssl_ctx`. Every TLS client connection leaks an `SSL` object and an `SSL_CTX`.
**2. `server_delete` leaks `SSL_CTX`**
`transport_tcp.c:52-57`
Same issue on the server side — `server_delete` closes the fd and frees the struct but never calls `SSL_CTX_free(server->ssl_ctx)`.
**3. `config_receive` doesn't initialize TLS fields**
`config.c:107-154`
When the server calls `config_receive()`, it does a raw `malloc` and only populates specific fields. `use_tls`, `tls_cert`, and `tls_key` are never initialized, leaving them as garbage. While nothing currently reads them on the server, this is technically undefined behavior.
**4. `tls_global_cleanup()` is never called anywhere**
`transport_tls.c:24-30`
Declared in the header but no code path invokes it. OpenSSL resources (error strings, algorithms) are leaked on exit. The global `g_ssl_ctx` is also never assigned to, making the cleanup dead code.
**5. Deprecated OpenSSL 1.0.x API**
`transport_tls.c:17-22,29`
`SSL_library_init()`, `OpenSSL_add_all_algorithms()`, `SSL_load_error_strings()`, `EVP_cleanup()` are all deprecated since OpenSSL 1.1.0 and removed in some OpenSSL 3.x builds. Since 1.1.0, initialization is automatic. These calls should be removed or guarded with `#if OPENSSL_VERSION_NUMBER < 0x10100000L`.
---
### Security Concerns
**6. No minimum TLS version enforced**
`transport_tls.c:33-34`
`create_ssl_ctx` doesn't call `SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION)`. On older systems, TLS 1.0 or 1.1 could be negotiated — both are deprecated and have known vulnerabilities.
**7. No server certificate verification on the client**
`transport_tls.c:32-61`
`create_ssl_ctx` for the client never calls `SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...)`. The client connects to the server without verifying its certificate, making it vulnerable to MITM attacks. This significantly undermines the value of TLS.
**8. SSL errors dumped to stderr**
`transport_tls.c:38,46,52,58,79`
`ERR_print_errors_fp(stderr)` leaks internal SSL error details (potentially including certificate paths, cipher info) to stdout. Should use the project's `log_message` system, or at minimum only log at debug level.
---
### Design Issues
**9. `tls_global_init()` called multiple times without guard**
`client_send.c:62,216`
Called in both `send_files()` and `send_chunks_multithreaded()`. While the underlying calls are idempotent, this is wasteful and should be called once at startup.
**10. Client allows NULL cert/key silently**
`transport_tls.c:41,149-150`
`create_ssl_ctx` has an `if (cert && key)` guard that silently creates a context without a client certificate. On the server side, `--tls` requires `--cert` and `--key`. This asymmetry is confusing — the user might think `--tls --cert cert.pem` works, but it silently skips loading the cert if `--key` isn't provided.
**11. `server_listen_tls` duplicates `server_listen`**
`transport_tls.c:93-134` vs `transport_tcp.c:59-89`
Nearly identical code (accept loop, fork, close fd). The only difference is the SSL handshake wrapper. Should share a common accept/fork loop or use a function pointer pattern.
**12. `-p` port not validated**
`server.c:177`
`atoi(argv[++i])` returns 0 on failure with no error checking. No range validation (1-65535).
---
### Minor / Style
**13. No server `--help` / usage message**
The server accepts `--stdio`, `--verbose`, `--tls`, `--cert`, `--key`, `-p` but has no help output.
**14. `__thread` inconsistency**
`protocol.c:9-11` — `io_read_fd` and `io_write_fd` are `__thread`, but `io_ssl` is plain `static`. This works because of the fork-per-client model (each child has its own copy), but it's inconsistent and fragile if the threading model changes.
---
### Summary
The most important fixes before merging:
1. Free `SSL`/`SSL_CTX` in `client_disconnect`/`client_delete`/`server_delete` (memory leaks)
2. Initialize TLS fields in `config_receive` (UB)
3. Add minimum TLS 1.2 version enforcement (security)
4. Add `SSL_VERIFY_PEER` on client + callback or flag (security)
5. Remove or gate deprecated OpenSSL API calls (portability)
NEW CRITICAL BUG: __thread on io_ssl breaks TLS in multithreaded server mode
protocol.c:11
io_ssl was changed from static SSL *io_ssl = NULL; to static __thread SSL *io_ssl = NULL;.
In the server's multithreaded path (-m):
tls_child_fn calls io_set_ssl(ssl) in the forked child's main thread
handler() creates receiver and writer threads via thrd_create
receiver thread calls receive_n_data() → checks io_ssl → NULL (each __thread variable is per-thread, initialized from the static initializer)
TLS is silently bypassed — read()/write() used instead of SSL_read()/SSL_write()
The client side is unaffected because client_connect_tls() and send_chunk() both run in the same thread (send_chunks_multithreaded).
Fix: Remove __thread from io_ssl, making it plain static. This is safe because:
Fork model (server): each child is a separate process with its own copy of static variables — no sharing issue
Thread model (client): only one thread sets/reads io_ssl (the sender thread), so no race condition
The __thread on io_read_fd/io_write_fd is fine — those are only used in the --stdio single-threaded path.
Minor remaining issues
1. atoi for port parsing server.c:195
atoi(argv[++i]) doesn't detect overflow (e.g., 99999999999). Use strtol with errno checking for robustness.
2. --ca without --tls
--ca is silently accepted even without --tls. The CA path sits unused in config. Minor, but could warn.
Summary
Must-fix:
Remove __thread from io_ssl — TLS is broken for server --tls -m (multithreaded mode)
## Re-review of 7a314b8
Great update — most of the original issues are resolved. Here's a summary:
### Fixed from previous review
- SSL/SSL cleanup in `client_disconnect`, `client_delete`, `server_delete` — done
- `config_receive` now initializes TLS fields — done
- Dead `g_ssl_ctx` and `tls_global_cleanup` removed — done
- Deprecated OpenSSL API guarded with `#if OPENSSL_VERSION_NUMBER` — done
- TLS 1.2 minimum enforced — done
- `SSL_VERIFY_PEER` support via `--ca` flag — done
- `ERR_print_errors_fp(stderr)` replaced with `log_ssl_errors()` using `log_message` — done
- `tls_global_init()` called once in main, removed from `send_files`/`send_chunks_multithreaded` — done
- Client validates `--tls` requires `--cert` and `--key` — done
- Shared `accept_loop` eliminates `server_listen`/`server_listen_tls` duplication — done
- Server `--help` added — done
- Port validation added — done
---
### NEW CRITICAL BUG: `__thread` on `io_ssl` breaks TLS in multithreaded server mode
`protocol.c:11`
`io_ssl` was changed from `static SSL *io_ssl = NULL;` to `static __thread SSL *io_ssl = NULL;`.
In the server's multithreaded path (`-m`):
1. `tls_child_fn` calls `io_set_ssl(ssl)` in the forked child's main thread
2. `handler()` creates `receiver` and `writer` threads via `thrd_create`
3. `receiver` thread calls `receive_n_data()` → checks `io_ssl` → **NULL** (each `__thread` variable is per-thread, initialized from the static initializer)
4. TLS is silently bypassed — `read()`/`write()` used instead of `SSL_read()`/`SSL_write()`
The client side is unaffected because `client_connect_tls()` and `send_chunk()` both run in the same thread (`send_chunks_multithreaded`).
**Fix:** Remove `__thread` from `io_ssl`, making it plain `static`. This is safe because:
- **Fork model (server):** each child is a separate process with its own copy of static variables — no sharing issue
- **Thread model (client):** only one thread sets/reads `io_ssl` (the sender thread), so no race condition
The `__thread` on `io_read_fd`/`io_write_fd` is fine — those are only used in the `--stdio` single-threaded path.
---
### Minor remaining issues
**1. `atoi` for port parsing**
`server.c:195`
`atoi(argv[++i])` doesn't detect overflow (e.g., `99999999999`). Use `strtol` with `errno` checking for robustness.
**2. `--ca` without `--tls`**
`--ca` is silently accepted even without `--tls`. The CA path sits unused in config. Minor, but could warn.
---
### Summary
Must-fix:
1. Remove `__thread` from `io_ssl` — TLS is broken for `server --tls -m` (multithreaded mode)
[1] src/shared/transport_tcp.c:51-56 — memory leak server_delete() does not free ssl_ctx. When TLS is used, server_create_tls() stores an SSL_CTX* in server->ssl_ctx, but server_delete() only closes the fd and frees the struct — the SSL_CTX leaks. Fix: Add to server_delete() before close:
[2] src/shared/transport_tcp.c:126-138 — memory leak + protocol violation client_disconnect() and client_delete() do not clean up ssl or ssl_ctx. This leaks the SSL session/context and fails to send TLS close_notify (peer sees abrupt TCP close). Fix: In client_disconnect(), before close(fd):
Note: ssl_ctx must be freed AFTER ssl since ssl was created from ssl_ctx.
[3] src/shared/config.c:101-149 — uninitialized fields config_receive() uses malloc(sizeof(Config)) (not calloc) and does NOT initialize the new fields: use_tls, tls_cert, tls_key. These will contain garbage values → undefined behavior. Fix: Add after config->min_size = 0:
protocol.c:10 — io_ssl is a plain global while io_read_fd/io_write_fd are __thread. Add __thread or document thread-safety contract.
transport_tls.c:17-21 — tls_global_init() ignores SSL_library_init() return; should check for failure.
transport_tls.c:15,25-29 — g_ssl_ctx is declared but never assigned; dead code. Remove it.
client_send.c:62,206 — No validation that both --cert and --key are provided when --tls is used. Silent cert ignore if one is missing.
transport_tls.c:17-21,25-29 — tls_global_cleanup() is never called anywhere. OpenSSL resources leaked on exit.
transport_tls.c:17-21 — SSL_library_init() etc. deprecated since OpenSSL 1.1.0; produces warnings on OpenSSL 3.x.
Style
protocol.h:12 — io_set_ssl() takes void *ssl instead of SSL *ssl; consider type safety.
server.c:169 — atoi() for port parsing silently returns 0 for non-numeric input; use strtol().
## PR Review Summary
**Verdict: FAIL** — 3 critical issues must be fixed before merge
**Files reviewed:** 13
**Issues found:** 11 (3 critical, 6 warning, 2 style)
---
### CRITICAL Issues
**[1] `src/shared/transport_tcp.c:51-56` — memory leak**
`server_delete()` does not free `ssl_ctx`. When TLS is used, `server_create_tls()` stores an `SSL_CTX*` in `server->ssl_ctx`, but `server_delete()` only closes the fd and frees the struct — the SSL_CTX leaks.
**Fix:** Add to `server_delete()` before close:
```c
if ((*server)->ssl_ctx) SSL_CTX_free((*server)->ssl_ctx);
```
**[2] `src/shared/transport_tcp.c:126-138` — memory leak + protocol violation**
`client_disconnect()` and `client_delete()` do not clean up `ssl` or `ssl_ctx`. This leaks the SSL session/context and fails to send TLS `close_notify` (peer sees abrupt TCP close).
**Fix:** In `client_disconnect()`, before `close(fd)`:
```c
if (client->ssl) { SSL_shutdown(client->ssl); SSL_free(client->ssl); client->ssl = NULL; }
```
In `client_delete()`, before `free(client)`:
```c
if (client->ssl_ctx) { SSL_CTX_free(client->ssl_ctx); client->ssl_ctx = NULL; }
```
Note: `ssl_ctx` must be freed AFTER `ssl` since ssl was created from ssl_ctx.
**[3] `src/shared/config.c:101-149` — uninitialized fields**
`config_receive()` uses `malloc(sizeof(Config))` (not `calloc`) and does NOT initialize the new fields: `use_tls`, `tls_cert`, `tls_key`. These will contain garbage values → undefined behavior.
**Fix:** Add after `config->min_size = 0`:
```c
config->use_tls = false;
config->tls_cert = NULL;
config->tls_key = NULL;
```
---
### Warnings
- **`protocol.c:10`** — `io_ssl` is a plain global while `io_read_fd`/`io_write_fd` are `__thread`. Add `__thread` or document thread-safety contract.
- **`transport_tls.c:17-21`** — `tls_global_init()` ignores `SSL_library_init()` return; should check for failure.
- **`transport_tls.c:15,25-29`** — `g_ssl_ctx` is declared but never assigned; dead code. Remove it.
- **`client_send.c:62,206`** — No validation that both `--cert` and `--key` are provided when `--tls` is used. Silent cert ignore if one is missing.
- **`transport_tls.c:17-21,25-29`** — `tls_global_cleanup()` is never called anywhere. OpenSSL resources leaked on exit.
- **`transport_tls.c:17-21`** — `SSL_library_init()` etc. deprecated since OpenSSL 1.1.0; produces warnings on OpenSSL 3.x.
### Style
- `protocol.h:12` — `io_set_ssl()` takes `void *ssl` instead of `SSL *ssl`; consider type safety.
- `server.c:169` — `atoi()` for port parsing silently returns 0 for non-numeric input; use `strtol()`.
[3] config_receive() initializes use_tls, tls_cert, tls_key (and new tls_ca) ✅
Warnings — all resolved or addressed
io_ssl no longer __thread✅
tls_global_init() now has deprecation guards for OpenSSL < 1.1.0 ✅
Dead g_ssl_ctx removed ✅
tls_global_cleanup() removed (was never called) ✅
--tls now validates both --cert and --key are provided ✅
Port parsing uses strtol() with range validation (1-65535) ✅
--ca option added for CA certificate verification ✅
Server now has --help and unknown option detection ✅
Accept loop refactored into shared accept_loop() — cleaner code ✅
Remaining minor items
protocol.h:12 — io_set_ssl() still takes void *ssl (acceptable, hides OpenSSL dep from header)
Safe to merge.
## Re-review after commits `7a314b8` + `df85509`
**Verdict: PASS** — All 3 critical issues fixed
### Critical issues — all resolved
- [1] `server_delete()` now frees `ssl_ctx` ✅
- [2] `client_disconnect()` calls `SSL_shutdown`/`SSL_free`; `client_delete()` frees `ssl_ctx` ✅
- [3] `config_receive()` initializes `use_tls`, `tls_cert`, `tls_key` (and new `tls_ca`) ✅
### Warnings — all resolved or addressed
- `io_ssl` no longer `__thread` ✅
- `tls_global_init()` now has deprecation guards for OpenSSL < 1.1.0 ✅
- Dead `g_ssl_ctx` removed ✅
- `tls_global_cleanup()` removed (was never called) ✅
- `--tls` now validates both `--cert` and `--key` are provided ✅
- Port parsing uses `strtol()` with range validation (1-65535) ✅
- `--ca` option added for CA certificate verification ✅
- Server now has `--help` and unknown option detection ✅
- Accept loop refactored into shared `accept_loop()` — cleaner code ✅
### Remaining minor items
- `protocol.h:12` — `io_set_ssl()` still takes `void *ssl` (acceptable, hides OpenSSL dep from header)
Safe to merge.
ff3e02e — SSL type safety + stale pointer prevention:
protocol.h: Added typedef struct ssl_st SSL; forward declaration — proper type safety for io_set_ssl() instead of void*✅
transport_tcp.c: Added io_set_ssl(NULL) in client_disconnect() — prevents stale SSL pointer access after disconnect ✅
017c119 — Use-after-free prevention:
transport_ssh.c: client->ssl = NULL; client->ssl_ctx = NULL; initialized in client_connect_ssh() — prevents SSL_free/SSL_CTX_free on uninitialized pointers if client_disconnect()/client_delete() is called on an SSH client ✅
Both are small, targeted, correct. No new issues.
Safe to merge.
## Re-review after commits `ff3e02e` + `017c119`
**Verdict: PASS** — Two good defensive fixes added
### New changes
**`ff3e02e` — SSL type safety + stale pointer prevention:**
- `protocol.h`: Added `typedef struct ssl_st SSL;` forward declaration — proper type safety for `io_set_ssl()` instead of `void*` ✅
- `transport_tcp.c`: Added `io_set_ssl(NULL)` in `client_disconnect()` — prevents stale SSL pointer access after disconnect ✅
**`017c119` — Use-after-free prevention:**
- `transport_ssh.c`: `client->ssl = NULL; client->ssl_ctx = NULL;` initialized in `client_connect_ssh()` — prevents `SSL_free`/`SSL_CTX_free` on uninitialized pointers if `client_disconnect()`/`client_delete()` is called on an SSH client ✅
Both are small, targeted, correct. No new issues.
Safe to merge.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
New TLS transport layer using OpenSSL for encrypted TCP connections.
Usage:
server: ./build/server --tls --cert server.pem --key server.pem -p 4433
client: ./build/client --tls --cert client.pem --key client.pem src dest
All 7 unit tests and 30 integration tests pass with zero warnings.
Code Review: TLS transport
Overall this is a solid first TLS implementation. The architecture is clean and the integration with existing transport abstractions is well thought out. Below are the issues I found, organized by severity.
CRITICAL / Bugs
1. Memory leaks:
SSLandSSL_CTXnot freed on client disconnect/deletetransport_tcp.c:129-141client_disconnect()only closes the fd and waits for SSH.client_delete()only frees the struct. Neither freesclient->sslorclient->ssl_ctx. Every TLS client connection leaks anSSLobject and anSSL_CTX.2.
server_deleteleaksSSL_CTXtransport_tcp.c:52-57Same issue on the server side —
server_deletecloses the fd and frees the struct but never callsSSL_CTX_free(server->ssl_ctx).3.
config_receivedoesn't initialize TLS fieldsconfig.c:107-154When the server calls
config_receive(), it does a rawmallocand only populates specific fields.use_tls,tls_cert, andtls_keyare never initialized, leaving them as garbage. While nothing currently reads them on the server, this is technically undefined behavior.4.
tls_global_cleanup()is never called anywheretransport_tls.c:24-30Declared in the header but no code path invokes it. OpenSSL resources (error strings, algorithms) are leaked on exit. The global
g_ssl_ctxis also never assigned to, making the cleanup dead code.5. Deprecated OpenSSL 1.0.x API
transport_tls.c:17-22,29SSL_library_init(),OpenSSL_add_all_algorithms(),SSL_load_error_strings(),EVP_cleanup()are all deprecated since OpenSSL 1.1.0 and removed in some OpenSSL 3.x builds. Since 1.1.0, initialization is automatic. These calls should be removed or guarded with#if OPENSSL_VERSION_NUMBER < 0x10100000L.Security Concerns
6. No minimum TLS version enforced
transport_tls.c:33-34create_ssl_ctxdoesn't callSSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION). On older systems, TLS 1.0 or 1.1 could be negotiated — both are deprecated and have known vulnerabilities.7. No server certificate verification on the client
transport_tls.c:32-61create_ssl_ctxfor the client never callsSSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, ...). The client connects to the server without verifying its certificate, making it vulnerable to MITM attacks. This significantly undermines the value of TLS.8. SSL errors dumped to stderr
transport_tls.c:38,46,52,58,79ERR_print_errors_fp(stderr)leaks internal SSL error details (potentially including certificate paths, cipher info) to stdout. Should use the project'slog_messagesystem, or at minimum only log at debug level.Design Issues
9.
tls_global_init()called multiple times without guardclient_send.c:62,216Called in both
send_files()andsend_chunks_multithreaded(). While the underlying calls are idempotent, this is wasteful and should be called once at startup.10. Client allows NULL cert/key silently
transport_tls.c:41,149-150create_ssl_ctxhas anif (cert && key)guard that silently creates a context without a client certificate. On the server side,--tlsrequires--certand--key. This asymmetry is confusing — the user might think--tls --cert cert.pemworks, but it silently skips loading the cert if--keyisn't provided.11.
server_listen_tlsduplicatesserver_listentransport_tls.c:93-134vstransport_tcp.c:59-89Nearly identical code (accept loop, fork, close fd). The only difference is the SSL handshake wrapper. Should share a common accept/fork loop or use a function pointer pattern.
12.
-pport not validatedserver.c:177atoi(argv[++i])returns 0 on failure with no error checking. No range validation (1-65535).Minor / Style
13. No server
--help/ usage messageThe server accepts
--stdio,--verbose,--tls,--cert,--key,-pbut has no help output.14.
__threadinconsistencyprotocol.c:9-11—io_read_fdandio_write_fdare__thread, butio_sslis plainstatic. This works because of the fork-per-client model (each child has its own copy), but it's inconsistent and fragile if the threading model changes.Summary
The most important fixes before merging:
SSL/SSL_CTXinclient_disconnect/client_delete/server_delete(memory leaks)config_receive(UB)SSL_VERIFY_PEERon client + callback or flag (security)Re-review of
7a314b8Great update — most of the original issues are resolved. Here's a summary:
Fixed from previous review
client_disconnect,client_delete,server_delete— doneconfig_receivenow initializes TLS fields — doneg_ssl_ctxandtls_global_cleanupremoved — done#if OPENSSL_VERSION_NUMBER— doneSSL_VERIFY_PEERsupport via--caflag — doneERR_print_errors_fp(stderr)replaced withlog_ssl_errors()usinglog_message— donetls_global_init()called once in main, removed fromsend_files/send_chunks_multithreaded— done--tlsrequires--certand--key— doneaccept_loopeliminatesserver_listen/server_listen_tlsduplication — done--helpadded — doneNEW CRITICAL BUG:
__threadonio_sslbreaks TLS in multithreaded server modeprotocol.c:11io_sslwas changed fromstatic SSL *io_ssl = NULL;tostatic __thread SSL *io_ssl = NULL;.In the server's multithreaded path (
-m):tls_child_fncallsio_set_ssl(ssl)in the forked child's main threadhandler()createsreceiverandwriterthreads viathrd_createreceiverthread callsreceive_n_data()→ checksio_ssl→ NULL (each__threadvariable is per-thread, initialized from the static initializer)read()/write()used instead ofSSL_read()/SSL_write()The client side is unaffected because
client_connect_tls()andsend_chunk()both run in the same thread (send_chunks_multithreaded).Fix: Remove
__threadfromio_ssl, making it plainstatic. This is safe because:io_ssl(the sender thread), so no race conditionThe
__threadonio_read_fd/io_write_fdis fine — those are only used in the--stdiosingle-threaded path.Minor remaining issues
1.
atoifor port parsingserver.c:195atoi(argv[++i])doesn't detect overflow (e.g.,99999999999). Usestrtolwitherrnochecking for robustness.2.
--cawithout--tls--cais silently accepted even without--tls. The CA path sits unused in config. Minor, but could warn.Summary
Must-fix:
__threadfromio_ssl— TLS is broken forserver --tls -m(multithreaded mode)PR Review Summary
Verdict: FAIL — 3 critical issues must be fixed before merge
Files reviewed: 13
Issues found: 11 (3 critical, 6 warning, 2 style)
CRITICAL Issues
[1]
src/shared/transport_tcp.c:51-56— memory leakserver_delete()does not freessl_ctx. When TLS is used,server_create_tls()stores anSSL_CTX*inserver->ssl_ctx, butserver_delete()only closes the fd and frees the struct — the SSL_CTX leaks.Fix: Add to
server_delete()before close:[2]
src/shared/transport_tcp.c:126-138— memory leak + protocol violationclient_disconnect()andclient_delete()do not clean upsslorssl_ctx. This leaks the SSL session/context and fails to send TLSclose_notify(peer sees abrupt TCP close).Fix: In
client_disconnect(), beforeclose(fd):In
client_delete(), beforefree(client):Note:
ssl_ctxmust be freed AFTERsslsince ssl was created from ssl_ctx.[3]
src/shared/config.c:101-149— uninitialized fieldsconfig_receive()usesmalloc(sizeof(Config))(notcalloc) and does NOT initialize the new fields:use_tls,tls_cert,tls_key. These will contain garbage values → undefined behavior.Fix: Add after
config->min_size = 0:Warnings
protocol.c:10—io_sslis a plain global whileio_read_fd/io_write_fdare__thread. Add__threador document thread-safety contract.transport_tls.c:17-21—tls_global_init()ignoresSSL_library_init()return; should check for failure.transport_tls.c:15,25-29—g_ssl_ctxis declared but never assigned; dead code. Remove it.client_send.c:62,206— No validation that both--certand--keyare provided when--tlsis used. Silent cert ignore if one is missing.transport_tls.c:17-21,25-29—tls_global_cleanup()is never called anywhere. OpenSSL resources leaked on exit.transport_tls.c:17-21—SSL_library_init()etc. deprecated since OpenSSL 1.1.0; produces warnings on OpenSSL 3.x.Style
protocol.h:12—io_set_ssl()takesvoid *sslinstead ofSSL *ssl; consider type safety.server.c:169—atoi()for port parsing silently returns 0 for non-numeric input; usestrtol().Re-review after commits
7a314b8+df85509Verdict: PASS — All 3 critical issues fixed
Critical issues — all resolved
server_delete()now freesssl_ctx✅client_disconnect()callsSSL_shutdown/SSL_free;client_delete()freesssl_ctx✅config_receive()initializesuse_tls,tls_cert,tls_key(and newtls_ca) ✅Warnings — all resolved or addressed
io_sslno longer__thread✅tls_global_init()now has deprecation guards for OpenSSL < 1.1.0 ✅g_ssl_ctxremoved ✅tls_global_cleanup()removed (was never called) ✅--tlsnow validates both--certand--keyare provided ✅strtol()with range validation (1-65535) ✅--caoption added for CA certificate verification ✅--helpand unknown option detection ✅accept_loop()— cleaner code ✅Remaining minor items
protocol.h:12—io_set_ssl()still takesvoid *ssl(acceptable, hides OpenSSL dep from header)Safe to merge.
b54d5eb102to017c119472Re-review after commits
ff3e02e+017c119Verdict: PASS — Two good defensive fixes added
New changes
ff3e02e— SSL type safety + stale pointer prevention:protocol.h: Addedtypedef struct ssl_st SSL;forward declaration — proper type safety forio_set_ssl()instead ofvoid*✅transport_tcp.c: Addedio_set_ssl(NULL)inclient_disconnect()— prevents stale SSL pointer access after disconnect ✅017c119— Use-after-free prevention:transport_ssh.c:client->ssl = NULL; client->ssl_ctx = NULL;initialized inclient_connect_ssh()— preventsSSL_free/SSL_CTX_freeon uninitialized pointers ifclient_disconnect()/client_delete()is called on an SSH client ✅Both are small, targeted, correct. No new issues.
Safe to merge.
017c119472to907baf3379