Skip to content

quic: Fix missing LHASH error checks in stream map & peer token cache - #31333

Closed
ngauder wants to merge 2 commits into
openssl:masterfrom
ngauder:lhash-error-checks
Closed

quic: Fix missing LHASH error checks in stream map & peer token cache#31333
ngauder wants to merge 2 commits into
openssl:masterfrom
ngauder:lhash-error-checks

Conversation

@ngauder

@ngauder ngauder commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR adds missing error checking guards to two LHASH insertion points in the QUIC stack.

  1. Stream Map Allocation (ssl/quic/quic_stream_map.c):
  • On insertion failure the newly allocated QUIC_STREAM is freed via OPENSSL_free and NULL is returned. If insertion failed, the newly allocated QUIC_STREAM object would leak and lookups would subsequently fail even though the stream was reported as allocated.
  1. Peer Token Caching (ssl/quic/quic_impl.c):
  • On failure the allocated token is freed using ossl_quic_free_peer_token(), the mutex is unlocked, and 0 is returned. This prevents a token object leak and accurately reports the failure status to the caller.
Checklist

@ngauder ngauder changed the title quic: Fix missing lhash error checks in stream map and token cache quic: Fix missing LHASH error checks in stream map and peer token cache May 29, 2026
@ngauder ngauder changed the title quic: Fix missing LHASH error checks in stream map and peer token cache quic: Fix missing LHASH error checks in stream map & peer token cache May 29, 2026
@ngauder
ngauder marked this pull request as ready for review May 29, 2026 13:33
Sashan
Sashan previously approved these changes May 29, 2026
View reviewed changes

@Sashan Sashan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. thanks.

@openssl-machine openssl-machine added the approval: review pending This pull request needs review by a committer label May 29, 2026
@bukka

bukka commented May 29, 2026

Copy link
Copy Markdown
Member

This looks correct

I got those actually noted together with few more unhundled cases:

### Apps
- openssl.c:587 - prog_init - lh_FUNCTION_insert

### Demos
- ossl-nghttp3.c:103 - h3_conn_create_stream - no insert failure handling

### Crypto

#### Conf
- conf_api.c:56 - _CONF_add_string - unhandled lh_CONF_VALUE_insert

#### Error
- err.c:256 - err_load_strings - unhandled lh_ERR_STRING_DATA_insert

#### X509
- v3_purp.c:493 - scan_ext_flags - unhandled OPENSSL_LH_insert

#### SSL

#### QUIC
- quic_impl.c:5407 - ossl_quic_set_peer_token - unhandled lh_QUIC_TOKEN_insert
- quic_stream_map.c:175 - ossl_quic_stream_map_alloc - unhandled lh_QUIC_STREAM_insert
- quic_record_tx.c:801 - ossl_qtx_write_pkt - qtx_get_cons_txe handling (see comment)

#### TLS
- ssl_cert.c:827 - SSL_load_client_CA_file_ex - unhandled lh_X509_NAME_insert
- ssl_cert.c:890 - add_file_cert_subjects_to_stack - unhandled lh_X509_NAME_insert
- ssl_cert.c:933 - SSL_add_file_cert_subjects_to_stack - unhandled lh_X509_NAME_insert
- ssl_cert.c:971 - SSL_add_dir_cert_subjects_to_stack - unhandled lh_X509_NAME_insert
- statem_clnt.c:3318 - tls_process_cert_status_body - unhandled sk_OCSP_RESPONSE_insert

It would be good to add mfail tests for those as well so it gets covered...

@ngauder

ngauder commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

I have added MFAIL test cases for the relevant functions. Thanks for the feedback

@openssl-machine openssl-machine added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jun 3, 2026
@bukka

bukka commented Jun 4, 2026

Copy link
Copy Markdown
Member

Just a note for whoever is going to merge this. The first commit can be cherry-picke to 3.5+ (only part of it (map alloc) applies to 3.4 so it cannot go there which is not probably a big deal). The second commit can go only to master.

This is btw. the last issue I see in my fuzz regression using mfail.

@openssl-machine

openssl-machine commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@nhorman nhorman added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jun 4, 2026
@npajkovsky npajkovsky added branch: master Applies to master branch branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 branch: 4.0 Applies to openssl-4.0 labels Jun 5, 2026
@esyr esyr added the triaged: bug The issue/pr is/fixes a bug label Jun 5, 2026
@t8m t8m added tests: exempted The PR is exempt from requirements for testing tests: present The PR has suitable tests present and removed tests: exempted The PR is exempt from requirements for testing labels Jun 8, 2026
openssl-machine pushed a commit that referenced this pull request Jun 10, 2026
@ngauder @jogme
quic: add lhash insert error checks
c8d08d7 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:23:37 2026
(Merged from #31333)
openssl-machine pushed a commit that referenced this pull request Jun 10, 2026
@ngauder @jogme
quic: Add MFAIL coverage for stream map allocation and token caching
6537100 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:23:38 2026
(Merged from #31333)
@jogme

jogme commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

3.4 branch does not contain the affected code

@jogme jogme removed the branch: 3.4 Applies to openssl-3.4 label Jun 10, 2026
openssl-machine pushed a commit that referenced this pull request Jun 10, 2026
@ngauder @jogme
quic: add lhash insert error checks
e1a7638 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:29:43 2026
(Merged from #31333)
openssl-machine pushed a commit that referenced this pull request Jun 10, 2026
@ngauder @jogme
quic: add lhash insert error checks
d01e152 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:28:41 2026
(Merged from #31333)
@jogme

jogme commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

as the mfail_tests are present only in master, the test coverage commit was not backported.

@jogme

jogme commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Merged to the labeled branches. Thank you for your contribution!

@jogme jogme closed this Jun 10, 2026
openssl-machine pushed a commit that referenced this pull request Jun 10, 2026
@ngauder @jogme
quic: add lhash insert error checks
8a45f6f 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:30:10 2026
(Merged from #31333)
@ngauder
ngauder deleted the lhash-error-checks branch June 14, 2026 18:31
n13l pushed a commit to n13l/openssl that referenced this pull request Jul 25, 2026
@ngauder @n13l
quic: add lhash insert error checks
9bdde4b 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:23:37 2026
(Merged from openssl#31333)
n13l pushed a commit to n13l/openssl that referenced this pull request Jul 25, 2026
@ngauder @n13l
quic: Add MFAIL coverage for stream map allocation and token caching
3a1c46d 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:23:38 2026
(Merged from openssl#31333)
rickyringler pushed a commit to rickyringler/openssl that referenced this pull request Aug 21, 2026
@ngauder Ricky Ringler
quic: add lhash insert error checks
584617e 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:23:37 2026
(Merged from openssl#31333)
rickyringler pushed a commit to rickyringler/openssl that referenced this pull request Aug 21, 2026
@ngauder Ricky Ringler
quic: Add MFAIL coverage for stream map allocation and token caching
36bf42c 
Reviewed-by: Nikola Pajkovsky <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
MergeDate: Wed Jun 10 11:23:38 2026
(Merged from openssl#31333)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nhorman nhorman nhorman approved these changes
@npajkovsky npajkovsky npajkovsky approved these changes
@Sashan Sashan Sashan left review comments

Assignees

No one assigned

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 branch: 4.0 Applies to openssl-4.0 tests: present The PR has suitable tests present triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ngauder @bukka @openssl-machine @jogme @Sashan @nhorman @npajkovsky @esyr @t8m