Fix `ssl-vendor` (OpenSSL) by joshuamegnauth54 · Pull Request #7985 · RustPython/RustPython

joshuamegnauth54 · 2026-05-26T18:46:11Z

Closes: #7893

Fix 1:
foreign-types-shared needs to match openssl's version. Bumping it is a SemVer violation because the latest versions of the crate aren't backwards compatible with older versions.
See: rust-openssl/rust-openssl#2461

Fix 2:
The second fix is to align the openssl module with the latest host_env and ssl changes.

Summary by CodeRabbit

Chores
- Updated OpenSSL-related workspace dependency with synchronization guidance
- Tightened internal API visibility for SSL components
CI
- Added an extra Linux build to include the vendored OpenSSL configuration
Refactor
- Improved SSL socket handling for better VM-aware behavior and more reliable handshake/read/write/shutdown operations

coderabbitai · 2026-05-26T18:46:26Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1fee12b9-5949-463f-b837-b7de35736695

📥 Commits

Reviewing files that changed from the base of the PR and between 128779a and eca26d1.

📒 Files selected for processing (4)

.github/workflows/ci.yaml
Cargo.toml
crates/stdlib/src/openssl.rs
crates/stdlib/src/ssl/error.rs

🚧 Files skipped from review as they are similar to previous changes (4)

.github/workflows/ci.yaml
Cargo.toml
crates/stdlib/src/ssl/error.rs
crates/stdlib/src/openssl.rs

📝 Walkthrough

Walkthrough

This PR threads a &VirtualMachine through OpenSSL socket readiness helpers, refactors selection to use sock_wait/SockWaitKind, updates all SSL handshake/read/write/shutdown call sites to pass the VM, restricts internal bio visibility, pins foreign-types-shared, and adds a CI step to build with ssl-vendor.

Changes

SSL VM-aware socket waiting

Layer / File(s)	Summary
Dependency version alignment & CI `Cargo.toml`, `.github/workflows/ci.yaml`	Pin workspace `foreign-types-shared` to `0.1` and add a Linux-only CI step building with `--features ssl-vendor`.
Import VM-aware wait helpers `crates/stdlib/src/openssl.rs`	Add `sock_wait` and `SockWaitKind` to `_ssl` module socket imports.
SocketStream::select refactor `crates/stdlib/src/openssl.rs`	`SocketStream::select` now accepts `vm: &VirtualMachine`, maps `SslNeeds` to `SockWaitKind`, computes timeout from `SocketDeadline`, waits via `sock_wait`, and returns `PyResult<SelectRet>`; `socket_needs` threads `vm` and returns `PyResult<(Option<SslNeeds>, SelectRet)>`.
Thread vm through SSL call sites `crates/stdlib/src/openssl.rs`	Handshake, write, read, and shutdown SSL loops now pass `vm` into `socket_needs`/`select` and propagate wait failures via `?`.
Internal bio visibility tightened `crates/stdlib/src/openssl.rs`	`bio::MemBioSlice` and its `new`/`as_ptr` functions changed from `pub` to `pub(super)`.
Conditional dead_code attrs for SSL errors `crates/stdlib/src/ssl/error.rs`	Add `#[cfg_attr(all(feature = "ssl-openssl", not(feature = "ssl-rustls")), expect(dead_code))]` to two SSL error constructor functions.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant PySslSocket
  participant SocketStream
  participant sock_wait
  participant VirtualMachine

  Client->>PySslSocket: read/write/shutdown
  PySslSocket->>SocketStream: select(SslNeeds, deadline, vm)
  SocketStream->>SocketStream: map SslNeeds -> SockWaitKind
  SocketStream->>sock_wait: sock_wait(vm, SockWaitKind, deadline)
  sock_wait->>VirtualMachine: request VM-aware wait
  VirtualMachine-->>sock_wait: wait result
  sock_wait-->>SocketStream: SelectRet
  SocketStream-->>PySslSocket: readiness result / error

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

RustPython/RustPython#7786: Adjusts dead-code lint handling in SSL error helpers (overlaps crates/stdlib/src/ssl/error.rs edits).

Suggested reviewers

ShaharNaveh
fanninpm

"🐰 I threaded VM through socket's song,
sock_wait hummed where waits belong,
deps pinned tight and CI sings along,
bio tucked in, errors blessed, not wrong,
hop, build, and test — SSL's dance is strong!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title "Fix `ssl-vendor` (OpenSSL)" clearly and specifically describes the main change—fixing compilation issues with the ssl-vendor feature for OpenSSL.
Linked Issues check	✅ Passed	All coding requirements from issue `#7893` are addressed: LazyLock/as_str compilation errors fixed, ssl-vendor builds restored, and openssl module aligned with host_env/ssl changes.
Out of Scope Changes check	✅ Passed	All changes directly address the ssl-vendor compilation failures: dependency version alignment, openssl module refactoring, CI coverage, and dead code suppression for alternate builds.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/stdlib/src/openssl.rs`:
- Around line 2319-2322: The match currently swallows errors from sock_wait;
change the code to propagate sock_wait errors instead of treating them as
SelectRet::Ok—call sock_wait(&*sock, wait_kind, timeout, vm)? (or otherwise map
Err to return the PyErr) and then match on the resulting bool to return
SelectRet::TimedOut or SelectRet::Ok so that errors (signals, EBADF, I/O) are
not discarded; update the block around sock_wait to use the ? operator or
explicit Err->return Err(...) propagation so exceptions bubble up instead of
being converted to SelectRet::Ok.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: c62f895b-4f7a-4004-93a8-8bfbec72063a

📥 Commits

Reviewing files that changed from the base of the PR and between dcb273b and d0b84cd.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

Cargo.toml
crates/stdlib/src/openssl.rs

ShaharNaveh

TYSM for doing this!

can you please add a somewhere in the CI something that checks it?

I think adding a cargo check --features ... under

RustPython/.github/workflows/ci.yaml

Lines 139 to 141 in dcb273b

    
           - name: Test openssl build 
        
             run: cargo build --no-default-features --features ssl-openssl 
        
             if: runner.os == 'Linux'

would be fine

joshuamegnauth54 · 2026-05-27T02:59:44Z

Sure. I'll handle it by tomorrow. 😸 I'll also handle CodeRabbit's lint. I don't know OpenSSL and TLS too well, but based on the context I think CodeRabbit is right that returning an error is the right call.

joshuamegnauth54 · 2026-05-27T16:56:04Z

Admittedly, I know very little about CI so I hope the CI change is right. 😹

I fixed the build but also changed select() to return an error to match sock_wait as CodeRabbit suggested.

ShaharNaveh

TYSM!

ShaharNaveh · 2026-05-27T17:20:08Z

Admittedly, I know very little about CI so I hope the CI change is right. 😹

dw, you did great:)

I fixed the build but also changed select() to return an error to match sock_wait as CodeRabbit suggested.

🔥

Closes: RustPython#7893 Fix 1: `foreign-types-shared` needs to match `openssl`'s version. Bumping it is a SemVer violation because the latest versions of the crate aren't backwards compatible with older versions. See: rust-openssl/rust-openssl#2461 Fix 2: The second fix is to align the `openssl` module with the latest `host_env` and `ssl` changes.

joshuamegnauth54 force-pushed the fix-ssl-vendor branch from f9de717 to d0b84cd Compare May 26, 2026 18:47

coderabbitai Bot reviewed May 26, 2026

View reviewed changes

Comment thread crates/stdlib/src/openssl.rs Outdated

ShaharNaveh requested changes May 26, 2026

View reviewed changes

joshuamegnauth54 force-pushed the fix-ssl-vendor branch from d0b84cd to 128779a Compare May 27, 2026 16:53

ShaharNaveh approved these changes May 27, 2026

View reviewed changes

youknowone force-pushed the fix-ssl-vendor branch from 128779a to eca26d1 Compare May 28, 2026 02:14

youknowone merged commit 7d54ba5 into RustPython:main May 28, 2026
26 checks passed

youknowone mentioned this pull request May 28, 2026

rename ssl-vendor to ssl-openssl-vendor #7989

Merged

joshuamegnauth54 deleted the fix-ssl-vendor branch May 29, 2026 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `ssl-vendor` (OpenSSL)#7985

Fix `ssl-vendor` (OpenSSL)#7985
youknowone merged 1 commit into
RustPython:mainfrom
joshuamegnauth54:fix-ssl-vendor

joshuamegnauth54 commented May 26, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 26, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

ShaharNaveh left a comment

Uh oh!

joshuamegnauth54 commented May 27, 2026

Uh oh!

joshuamegnauth54 commented May 27, 2026

Uh oh!

ShaharNaveh left a comment

Uh oh!

ShaharNaveh commented May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	- name: Test openssl build
	run: cargo build --no-default-features --features ssl-openssl
	if: runner.os == 'Linux'

Conversation

joshuamegnauth54 commented May 26, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

joshuamegnauth54 commented May 27, 2026

Uh oh!

joshuamegnauth54 commented May 27, 2026

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh commented May 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshuamegnauth54 commented May 26, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 26, 2026 •

edited

Loading