Skip to content

Commit d750782

Browse files
nanangizznanangizz
committed
Refactor SSL socket send path and improve test coverage
Replace the ring-buffer-based send mechanism (send_buf + single send_buf_pending slot) with per-op pool-allocated ssl_send_op_t using embedded encrypted data buffer, free list with configurable cap, and true memory release on discard. This eliminates the PJ_ENOMEM crash during renegotiation reported in #4878. Key changes: - New ssl_send_op_t with per-op pool for true memory release - ssl_do_handshake_and_flush() wrapper unifying error handling across all 5 SSL backends (OpenSSL, GnuTLS, mbedTLS, Schannel, Darwin) - Fix flush_delayed_send: pass correct app_key, handle PJ_EPENDING to prevent double-send, invoke callback on synchronous success - Fix ssock_on_data_sent: check app_key (not send_key) for handshake/shutdown detection (was dead code before) - Fix ssock_on_connect_complete: return via on_handshake_complete instead of bare PJ_ENOMEM from pj_bool_t function - Rename circ_buf_output/input to ssl_write_buf/ssl_read_buf for clarity (not always circular — OpenSSL uses memory BIO) - Add "Caller must hold write_mutex" documentation to ssl_write() in all 6 backends Test improvements: - send_load_test: 200 rapid sends with async callback verification - large_msg_test: 64KB message (multi-TLS-record) echo verification - close_pending_test: close socket with pending sends (no crash) - bidir_test: bidirectional simultaneous send load - mt_send_load_test: 3 worker threads + 3 concurrent clients CI: add ioq-no-fast-track job (PJ_IOQUEUE_FAST_TRACK=0 + ASan) to force async send path in activesock and ssl_sock tests. Co-Authored-By: Claude Code
1 parent af42c36 commit d750782

18 files changed

+2120
-463
lines changed

‎.github/workflows/ci-linux.yml‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,57 @@ jobs:
545545
name: ${{ runner.os }}-${{ runner.arch }}-${{ github.job }}-${{ github.run_id }}
546546
path: artifacts
547547

548+
ioq-no-fast-track:
549+
needs: [detect-changes]
550+
if: github.event_name == 'push' || needs.detect-changes.outputs.pjlib_deps == 'true'
551+
runs-on: ubuntu-latest
552+
strategy:
553+
fail-fast: false
554+
matrix:
555+
include:
556+
- name: select+ossl
557+
config_site: |
558+
#define PJ_TODO(x)
559+
#define PJ_IOQUEUE_FAST_TRACK 0
560+
#define PJ_IOQUEUE_IMP PJ_IOQUEUE_IMP_SELECT
561+
configure_args: ""
562+
install_deps: ""
563+
- name: epoll+gtls
564+
config_site: |
565+
#define PJ_TODO(x)
566+
#define PJ_IOQUEUE_FAST_TRACK 0
567+
configure_args: "--with-gnutls=/usr/"
568+
install_deps: "sudo apt-get update && sudo apt-get install -y libgnutls28-dev"
569+
name: ioq-no-fast-track / ${{ matrix.name }}
570+
steps:
571+
- uses: actions/checkout@v2
572+
- name: install cirunner
573+
run: |
574+
git clone --depth 1 https://github.com/pjsip/cirunner.git
575+
cirunner/installlinux.sh
576+
- name: install dependencies
577+
if: matrix.install_deps != ''
578+
run: ${{ matrix.install_deps }}
579+
- name: config site
580+
run: |
581+
cat > pjlib/include/pj/config_site.h << 'SITE_EOF'
582+
${{ matrix.config_site }}
583+
SITE_EOF
584+
- name: configure
585+
run: CFLAGS="-g -fsanitize=address" LDFLAGS="-rdynamic -fsanitize=address" ./configure ${{ matrix.configure_args }}
586+
- name: make
587+
run: $MAKE_FAST
588+
- name: pjlib ioqueue, activesock, ssl_sock test
589+
run: |
590+
export LSAN_OPTIONS="suppressions=$GITHUB_WORKSPACE/tests/sanitizers/lsan.supp"
591+
cd pjlib/build && ../bin/pjlib-test-`make -s -C ../.. infotarget` $CI_MODE $CI_ARGS udp_ioqueue_test tcp_ioqueue_test udp_ioqueue_unreg_test activesock_test ssl_sock_test ssl_sock_stress_test
592+
- name: upload artifacts on failure
593+
if: ${{ failure() }}
594+
uses: actions/upload-artifact@v4
595+
with:
596+
name: ${{ runner.os }}-${{ runner.arch }}-${{ github.job }}-${{ matrix.name }}-${{ github.run_id }}
597+
path: artifacts
598+
548599
cmake-build:
549600
runs-on: ubuntu-latest
550601
name: CMake / ${{ matrix.build_type }}

‎pjlib/CMakeLists.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -729,6 +729,7 @@ if(BUILD_TESTING)
729729
src/pjlib-test/sock.c
730730
src/pjlib-test/sock_perf.c
731731
src/pjlib-test/ssl_sock.c
732+
src/pjlib-test/ssl_sock_stress.c
732733
src/pjlib-test/string.c
733734
src/pjlib-test/test.c
734735
src/pjlib-test/thread.c

‎pjlib/build/Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export TEST_OBJS += activesock.o atomic.o atomic_slist.o \
5151
fifobuf.o file.o hash_test.o ioq_perf.o ioq_udp.o \
5252
ioq_stress_test.o ioq_unreg.o ioq_tcp.o ioq_iocp_unreg_test.o \
5353
list.o mutex.o os.o pool.o pool_perf.o rand.o rbtree.o \
54-
select.o sleep.o sock.o sock_perf.o ssl_sock.o \
54+
select.o sleep.o sock.o sock_perf.o ssl_sock.o ssl_sock_stress.o \
5555
string.o test.o thread.o timer.o timestamp.o \
5656
udp_echo_srv_sync.o udp_echo_srv_ioqueue.o \
5757
unittest_test.o util.o

‎pjlib/build/pjlib_test.vcxproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -789,6 +789,7 @@
789789
<ClCompile Include="..\src\pjlib-test\sock.c" />
790790
<ClCompile Include="..\src\pjlib-test\sock_perf.c" />
791791
<ClCompile Include="..\src\pjlib-test\ssl_sock.c" />
792+
<ClCompile Include="..\src\pjlib-test\ssl_sock_stress.c" />
792793
<ClCompile Include="..\src\pjlib-test\atomic_slist.c" />
793794
<ClCompile Include="..\src\pjlib-test\string.c" />
794795
<ClCompile Include="..\src\pjlib-test\test.c" />

‎pjlib/build/pjlib_test.vcxproj.filters‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,9 @@
9393
<ClCompile Include="..\src\pjlib-test\ssl_sock.c">
9494
<Filter>Source Files</Filter>
9595
</ClCompile>
96+
<ClCompile Include="..\src\pjlib-test\ssl_sock_stress.c">
97+
<Filter>Source Files</Filter>
98+
</ClCompile>
9699
<ClCompile Include="..\src\pjlib-test\string.c">
97100
<Filter>Source Files</Filter>
98101
</ClCompile>

‎pjlib/src/pj/ssl_sock_apple.m‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,12 +1135,12 @@ static pj_status_t network_setup_connection(pj_ssl_sock_t *ssock,
11351135
pj_status_t status;
11361136

11371137
/* Initialize input circular buffer */
1138-
status = circ_init(ssock->pool->factory, &ssock->circ_buf_input, 8192);
1138+
status = circ_init(ssock->pool->factory, &ssock->ssl_read_buf, 8192);
11391139
if (status != PJ_SUCCESS)
11401140
return status;
11411141

11421142
/* Initialize output circular buffer */
1143-
status = circ_init(ssock->pool->factory, &ssock->circ_buf_output, 8192);
1143+
status = circ_init(ssock->pool->factory, &ssock->ssl_write_buf, 8192);
11441144
if (status != PJ_SUCCESS)
11451145
return status;
11461146

@@ -1525,8 +1525,8 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
15251525
}
15261526

15271527
/* Destroy circular buffers */
1528-
circ_deinit(&ssock->circ_buf_input);
1529-
circ_deinit(&ssock->circ_buf_output);
1528+
circ_deinit(&ssock->ssl_read_buf);
1529+
circ_deinit(&ssock->ssl_write_buf);
15301530

15311531
PJ_LOG(4, (THIS_FILE, "SSL %p destroyed", ssock));
15321532
}
@@ -1535,9 +1535,9 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
15351535
/* Reset socket state. */
15361536
static void ssl_reset_sock_state(pj_ssl_sock_t *ssock)
15371537
{
1538-
pj_lock_acquire(ssock->circ_buf_output_mutex);
1538+
pj_lock_acquire(ssock->ssl_write_buf_mutex);
15391539
ssock->ssl_state = SSL_STATE_NULL;
1540-
pj_lock_release(ssock->circ_buf_output_mutex);
1540+
pj_lock_release(ssock->ssl_write_buf_mutex);
15411541

15421542
#if SSL_DEBUG
15431543
PJ_LOG(3, (THIS_FILE, "SSL reset sock state %p", ssock));
@@ -2170,20 +2170,20 @@ static pj_status_t ssl_read(pj_ssl_sock_t *ssock, void *data, int *size)
21702170
{
21712171
pj_size_t circ_buf_size, read_size;
21722172

2173-
pj_lock_acquire(ssock->circ_buf_input_mutex);
2173+
pj_lock_acquire(ssock->ssl_read_buf_mutex);
21742174

2175-
if (circ_empty(&ssock->circ_buf_input)) {
2176-
pj_lock_release(ssock->circ_buf_input_mutex);
2175+
if (circ_empty(&ssock->ssl_read_buf)) {
2176+
pj_lock_release(ssock->ssl_read_buf_mutex);
21772177
*size = 0;
21782178
return PJ_SUCCESS;
21792179
}
21802180

2181-
circ_buf_size = circ_size(&ssock->circ_buf_input);
2181+
circ_buf_size = circ_size(&ssock->ssl_read_buf);
21822182
read_size = PJ_MIN(circ_buf_size, (pj_size_t)*size);
21832183

2184-
circ_read(&ssock->circ_buf_input, data, read_size);
2184+
circ_read(&ssock->ssl_read_buf, data, read_size);
21852185

2186-
pj_lock_release(ssock->circ_buf_input_mutex);
2186+
pj_lock_release(ssock->ssl_read_buf_mutex);
21872187

21882188
*size = read_size;
21892189

@@ -2192,14 +2192,14 @@ static pj_status_t ssl_read(pj_ssl_sock_t *ssock, void *data, int *size)
21922192

21932193
/*
21942194
* Write the plain data to buffer. It will be encrypted later during
2195-
* sending.
2195+
* sending. Caller must hold ssock->write_mutex.
21962196
*/
21972197
static pj_status_t ssl_write(pj_ssl_sock_t *ssock, const void *data,
21982198
pj_ssize_t size, int *nwritten)
21992199
{
22002200
pj_status_t status;
22012201

2202-
status = circ_write(&ssock->circ_buf_output, data, size);
2202+
status = circ_write(&ssock->ssl_write_buf, data, size);
22032203
*nwritten = (status == PJ_SUCCESS)? (int)size: 0;
22042204

22052205
return status;

‎pjlib/src/pj/ssl_sock_darwin.c‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ static OSStatus SocketWrite(SSLConnectionRef connection,
109109
pj_size_t len = *dataLength;
110110

111111
pj_lock_acquire(ssock->write_mutex);
112-
if (circ_write(&ssock->circ_buf_output, data, len) != PJ_SUCCESS) {
112+
if (circ_write(&ssock->ssl_write_buf, data, len) != PJ_SUCCESS) {
113113
pj_lock_release(ssock->write_mutex);
114114
*dataLength = 0;
115115
return errSSLInternal;
@@ -128,22 +128,22 @@ static OSStatus SocketRead(SSLConnectionRef connection,
128128
pj_ssl_sock_t *ssock = (pj_ssl_sock_t *)connection;
129129
pj_size_t len = *dataLength;
130130

131-
pj_lock_acquire(ssock->circ_buf_input_mutex);
131+
pj_lock_acquire(ssock->ssl_read_buf_mutex);
132132

133-
if (circ_empty(&ssock->circ_buf_input)) {
134-
pj_lock_release(ssock->circ_buf_input_mutex);
133+
if (circ_empty(&ssock->ssl_read_buf)) {
134+
pj_lock_release(ssock->ssl_read_buf_mutex);
135135

136136
/* Data buffers not yet filled */
137137
*dataLength = 0;
138138
return errSSLWouldBlock;
139139
}
140140

141-
pj_size_t circ_buf_size = circ_size(&ssock->circ_buf_input);
141+
pj_size_t circ_buf_size = circ_size(&ssock->ssl_read_buf);
142142
pj_size_t read_size = PJ_MIN(circ_buf_size, len);
143143

144-
circ_read(&ssock->circ_buf_input, data, read_size);
144+
circ_read(&ssock->ssl_read_buf, data, read_size);
145145

146-
pj_lock_release(ssock->circ_buf_input_mutex);
146+
pj_lock_release(ssock->ssl_read_buf_mutex);
147147

148148
*dataLength = read_size;
149149

@@ -377,12 +377,12 @@ static pj_status_t ssl_create(pj_ssl_sock_t *ssock)
377377
pj_status_t status;
378378

379379
/* Initialize input circular buffer */
380-
status = circ_init(ssock->pool->factory, &ssock->circ_buf_input, 8192);
380+
status = circ_init(ssock->pool->factory, &ssock->ssl_read_buf, 8192);
381381
if (status != PJ_SUCCESS)
382382
return status;
383383

384384
/* Initialize output circular buffer */
385-
status = circ_init(ssock->pool->factory, &ssock->circ_buf_output, 8192);
385+
status = circ_init(ssock->pool->factory, &ssock->ssl_write_buf, 8192);
386386
if (status != PJ_SUCCESS)
387387
return status;
388388

@@ -511,17 +511,17 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
511511
}
512512

513513
/* Destroy circular buffers */
514-
circ_deinit(&ssock->circ_buf_input);
515-
circ_deinit(&ssock->circ_buf_output);
514+
circ_deinit(&ssock->ssl_read_buf);
515+
circ_deinit(&ssock->ssl_write_buf);
516516
}
517517

518518

519519
/* Reset socket state. */
520520
static void ssl_reset_sock_state(pj_ssl_sock_t *ssock)
521521
{
522-
pj_lock_acquire(ssock->circ_buf_output_mutex);
522+
pj_lock_acquire(ssock->ssl_write_buf_mutex);
523523
ssock->ssl_state = SSL_STATE_NULL;
524-
pj_lock_release(ssock->circ_buf_output_mutex);
524+
pj_lock_release(ssock->ssl_write_buf_mutex);
525525

526526
ssl_close_sockets(ssock);
527527
}
@@ -1366,11 +1366,6 @@ static pj_status_t ssl_do_handshake(pj_ssl_sock_t *ssock)
13661366
}
13671367
pj_lock_release(ssock->write_mutex);
13681368

1369-
status = flush_circ_buf_output(ssock, &ssock->handshake_op_key, 0, 0);
1370-
if (status != PJ_SUCCESS && status != PJ_EPENDING) {
1371-
return status;
1372-
}
1373-
13741369
if (ret == noErr) {
13751370
/* Handshake has been completed */
13761371
ssock->ssl_state = SSL_STATE_ESTABLISHED;
@@ -1402,6 +1397,7 @@ static pj_status_t ssl_read(pj_ssl_sock_t *ssock, void *data, int *size)
14021397
/*
14031398
* Write the plain data to Darwin SSL, it will be encrypted by SSLWrite()
14041399
* and call SocketWrite.
1400+
* Caller must hold ssock->write_mutex.
14051401
*/
14061402
static pj_status_t ssl_write(pj_ssl_sock_t *ssock, const void *data,
14071403
pj_ssize_t size, int *nwritten)

‎pjlib/src/pj/ssl_sock_gtls.c‎

Lines changed: 17 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -356,15 +356,15 @@ static pj_ssize_t tls_data_push(gnutls_transport_ptr_t ptr,
356356
pj_ssl_sock_t *ssock = (pj_ssl_sock_t *)ptr;
357357
gnutls_sock_t *gssock = (gnutls_sock_t *)ssock;
358358

359-
pj_lock_acquire(ssock->circ_buf_output_mutex);
360-
if (circ_write(&ssock->circ_buf_output, data, len) != PJ_SUCCESS) {
361-
pj_lock_release(ssock->circ_buf_output_mutex);
359+
pj_lock_acquire(ssock->ssl_write_buf_mutex);
360+
if (circ_write(&ssock->ssl_write_buf, data, len) != PJ_SUCCESS) {
361+
pj_lock_release(ssock->ssl_write_buf_mutex);
362362

363363
gnutls_transport_set_errno(gssock->session, ENOMEM);
364364
return -1;
365365
}
366366

367-
pj_lock_release(ssock->circ_buf_output_mutex);
367+
pj_lock_release(ssock->ssl_write_buf_mutex);
368368

369369
return len;
370370
}
@@ -378,22 +378,22 @@ static pj_ssize_t tls_data_pull(gnutls_transport_ptr_t ptr,
378378
pj_ssl_sock_t *ssock = (pj_ssl_sock_t *)ptr;
379379
gnutls_sock_t *gssock = (gnutls_sock_t *)ssock;
380380

381-
pj_lock_acquire(ssock->circ_buf_input_mutex);
381+
pj_lock_acquire(ssock->ssl_read_buf_mutex);
382382

383-
if (circ_empty(&ssock->circ_buf_input)) {
384-
pj_lock_release(ssock->circ_buf_input_mutex);
383+
if (circ_empty(&ssock->ssl_read_buf)) {
384+
pj_lock_release(ssock->ssl_read_buf_mutex);
385385

386386
/* Data buffers not yet filled */
387387
gnutls_transport_set_errno(gssock->session, EAGAIN);
388388
return -1;
389389
}
390390

391-
pj_size_t circ_buf_size = circ_size(&ssock->circ_buf_input);
391+
pj_size_t circ_buf_size = circ_size(&ssock->ssl_read_buf);
392392
pj_size_t read_size = PJ_MIN(circ_buf_size, len);
393393

394-
circ_read(&ssock->circ_buf_input, data, read_size);
394+
circ_read(&ssock->ssl_read_buf, data, read_size);
395395

396-
pj_lock_release(ssock->circ_buf_input_mutex);
396+
pj_lock_release(ssock->ssl_read_buf_mutex);
397397

398398
return read_size;
399399
}
@@ -680,12 +680,12 @@ static pj_status_t ssl_create(pj_ssl_sock_t *ssock)
680680
(gnutls_transport_ptr_t) (uintptr_t) ssock);
681681

682682
/* Initialize input circular buffer */
683-
status = circ_init(ssock->pool->factory, &ssock->circ_buf_input, 512);
683+
status = circ_init(ssock->pool->factory, &ssock->ssl_read_buf, 512);
684684
if (status != PJ_SUCCESS)
685685
return status;
686686

687687
/* Initialize output circular buffer */
688-
status = circ_init(ssock->pool->factory, &ssock->circ_buf_output, 512);
688+
status = circ_init(ssock->pool->factory, &ssock->ssl_write_buf, 512);
689689
if (status != PJ_SUCCESS)
690690
return status;
691691

@@ -849,17 +849,17 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
849849
}
850850

851851
/* Destroy circular buffers */
852-
circ_deinit(&ssock->circ_buf_input);
853-
circ_deinit(&ssock->circ_buf_output);
852+
circ_deinit(&ssock->ssl_read_buf);
853+
circ_deinit(&ssock->ssl_write_buf);
854854
}
855855

856856

857857
/* Reset socket state. */
858858
static void ssl_reset_sock_state(pj_ssl_sock_t *ssock)
859859
{
860-
pj_lock_acquire(ssock->circ_buf_output_mutex);
860+
pj_lock_acquire(ssock->ssl_write_buf_mutex);
861861
ssock->ssl_state = SSL_STATE_NULL;
862-
pj_lock_release(ssock->circ_buf_output_mutex);
862+
pj_lock_release(ssock->ssl_write_buf_mutex);
863863

864864
ssl_close_sockets(ssock);
865865

@@ -1157,10 +1157,6 @@ static pj_status_t ssl_do_handshake(pj_ssl_sock_t *ssock)
11571157
/* Perform SSL handshake */
11581158
ret = gnutls_handshake(gssock->session);
11591159

1160-
status = flush_circ_buf_output(ssock, &ssock->handshake_op_key, 0, 0);
1161-
if (status != PJ_SUCCESS)
1162-
return status;
1163-
11641160
if (ret == GNUTLS_E_SUCCESS) {
11651161
/* System are GO */
11661162
ssock->ssl_state = SSL_STATE_ESTABLISHED;
@@ -1210,6 +1206,7 @@ static pj_status_t ssl_read(pj_ssl_sock_t *ssock, void *data, int *size)
12101206
* Write the plain data to GnuTLS, it will be encrypted by gnutls_record_send()
12111207
* and sent via tls_data_push. Note that re-negotitation may be on progress, so
12121208
* sending data should be delayed until re-negotiation is completed.
1209+
* Caller must hold ssock->write_mutex.
12131210
*/
12141211
static pj_status_t ssl_write(pj_ssl_sock_t *ssock, const void *data,
12151212
pj_ssize_t size, int *nwritten)

0 commit comments

Comments
 (0)