Skip to content

[2.7.18.16] Review follow-ups: _socket debug link, SNI None ref, sddl.h - #111

Merged
icanhasmath merged 2 commits into
2.7.18.16from
2.7.18.16-review-followups
Oct 28, 2026
Merged

icanhasmath merged 2 commits into
2.7.18.16from
2.7.18.16-review-followups

Conversation

@icanhasmath

Copy link
Copy Markdown

Small follow-ups from the reviews of #106 and #108, for 2.7.18.16. Linear PROD-1588.

_socket doesn't link in unoptimized builds (the #106 review's FYI)

  • socketmodule.c has a socket_parse_timeout() that nothing calls. It arrived with the CVE-2023-40217 getblocking() backport (4737406, first released in 2.7.18.10, and in 2.7.18.14).
  • It calls _PyTime_FromSecondsObject(), but 2.7 never compiles Python/pytime.c.
  • -O2 builds drop the unused static function, which is why release builds work. An -O0 or --with-pydebug build leaves _PyTime_FromSecondsObject undefined in _socket, so neither _socket nor ssl can be imported.
  • Fix: remove the function. Compiling socketmodule.c at -O0 now leaves no undefined _PyTime_FromSecondsObject; before, there was one.

_ssl.c: unbalanced Py_DECREF(None) (the #106 review's ⚪)

posixmodule.c: <sddl.h> under MS_WINDOWS (the #108 review's ⚪)

  • This puts the include in the same scope as the helpers that use it, as upstream does.

Correction: #108's commit message cites eb29e2f590 as upstream's pythongh-118486 commit. The original main commit is 81939dad77 (pythonGH-118488), as the review says. That merged message can't be edited, so this commit's message records the correction.

Testing (Linux x86_64, OpenSSL 1.1.1w):

  • test_ssl: the only failure is the network-dependent test_context_setget, as on the base. That includes both SNI release tests.
  • test_os, test_tempfile and test_posix pass.
  • Windows: not built.

Not in this PR: while checking _socket, I found that Lib/test/test_socket.py is broken on this line, and has been in every release since an earlier partial 3.x backport. It uses fcntl and multiprocessing without importing them, so the module fails to load and none of its tests run. Fixing that is a separate test-only job: once the imports are there, the backported tests themselves need checking.

…dl.h

Three small fixes raised in the reviews of #106 and #108:

- socketmodule.c: remove socket_parse_timeout().  It came in with the
  CVE-2023-40217 getblocking() backport (4737406, first released in
  2.7.18.10), has no callers, and calls _PyTime_FromSecondsObject(),
  which 2.7 never builds (Python/pytime.c is not compiled).  Optimized
  builds drop the unused static function, but an -O0 or --with-pydebug
  build leaves an undefined symbol in _socket, so _socket (and ssl)
  fail to import.

- _ssl.c: _servername_callback() set ssl_socket to Py_None without a
  reference when the SSL object has no Python socket, and the error path
  then did Py_DECREF(ssl_socket).  Take the reference.  This predates
  #106; it is not reachable through ssl.SSLSocket, which always sets
  ssl_sock.

- posixmodule.c: include <sddl.h> under MS_WINDOWS, as upstream does,
  instead of inside the _MSC_VER block, so it matches the MS_WINDOWS
  helpers that call ConvertStringSecurityDescriptorToSecurityDescriptorW
  (#108).

Correction to the #108 commit message (386508c): it cites upstream's
pythongh-118486 commit as eb29e2f.  The original main commit is
81939da (pythonGH-118488, 2024-05-02), followed by 8ed5466 (OWNER
RIGHTS) and 8af84b5 (pythongh-118773, the SDDL string the port uses).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@ezequielp-activestate ezequielp-activestate left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No blockers. Built at -O0 --with-pydebug on Python 2.7 with OpenSSL 1.1.1d:

  • _socket: removing the unused socket_parse_timeout() fixes the unoptimized build. _socket.so has no undefined _PyTime_FromSecondsObject, and _socket and ssl both import. Nothing else in socketmodule.c used it. The remaining _PyTime_FloatTime calls are 2.7's own.
  • _servername_callback: the new Py_INCREF(Py_None) balances every exit. On the no-socket path, the only release of ssl_socket is the one in error:, which it always reaches.
  • test_ssl: 103 tests pass on this debug build (14 skipped), including all the SNI callback tests and the two context-released ones.
  • <sddl.h>: it now sits under MS_WINDOWS, beside the helpers that use it, as upstream has it.

Test notes: test_os and test_tempfile pass. test_posix's three chown tests error only because the build ran as root in a rootless container. test_ssl imports the third-party mock package, which isn't in the stdlib on 2.7, so a plain build can't run it without mock installed. Not built on Windows.

Review follow-up (#111).  test_ssl.py imported "from mock import Mock"
(added with an earlier ssl backport, 37df9f7, and in 2.7.18.14), but
mock is not part of the 2.7 standard library, so on a plain build the
whole module failed with "ImportError: No module named mock" and none
of test_ssl ran.  Mock itself was unused; the only mock use,
mock.patch.dict(os.environ) in test_keylog_env, referred to a name the
file never imported.  Use support.EnvironmentVarGuard there instead and
remove the import.  (test_keylog_env is skipped on 2.7, whose _ssl has
no keylog callback.)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@icanhasmath

Copy link
Copy Markdown
Author

Thanks. Your mock note was a real gap, so I've fixed it here in b9b77db:

  • Cause: Lib/test/test_ssl.py had from mock import Mock, added with an earlier ssl backport (37df9f7) and already in 2.7.18.14. Without the third-party package, import test.test_ssl fails, so a plain build ran none of test_ssl.
  • Mock itself was never used. The only mock use, mock.patch.dict(os.environ) in test_keylog_env, referenced a name the file never imported.
  • Fix: that test now uses support.EnvironmentVarGuard(), and the import is gone.
  • Result: without mock installed, test_ssl imports and runs 117 tests, 12 skipped. The only error is the network-dependent test_context_setget. test_keylog_env is skipped on 2.7 because _ssl has no keylog callback, so I exercised the rewritten block directly: it sets SSLKEYLOGFILE and restores the environment afterwards.
  • No change needed: the test_posix chown errors come from running as root in the container.

This commit is new since your approval, so could you take a quick look before it merges?

@ezequielp-activestate ezequielp-activestate left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review of b9b77db: still approved. test_ssl no longer imports the third-party mock: its only use, in test_keylog_env, is now support.EnvironmentVarGuard, and nothing else in the file refers to mock/Mock. On a fresh 2.7 build with no mock installed (import mock fails), python -m test test_ssl runs 103 tests, OK (14 skipped).

⚪ FYI, pre-existing and not for this PR: TestSSLDebug isn't in test_ssl.test_main's list, so regrtest never runs it. Running it directly, its three keylog tests (including test_keylog_env) skip, because 2.7's SSLContext has no keylog_filename. The other three error with 'SSLContext' object has no attribute '_msg_callback'. It belongs with the test_socket import problem you noted, in a later test-only cleanup.

@icanhasmath
icanhasmath merged commit 0471d8e into 2.7.18.16 Oct 28, 2026
icanhasmath added a commit that referenced this pull request Oct 28, 2026
Bump PY_VERSION to 2.7.18.16 and add release notes for the security
fixes merged into 2.7.18.16 (#92-#108, #111): tarfile (CVE-2026-11972),
csv.Sniffer (CVE-2026-18503), ftplib.ftpcp (CVE-2026-8328), httplib
(CVE-2026-103995, CVE-2021-3737 and GHSA-w4q2-g22w-6fr4),
SimpleHTTPServer (CVE-2021-28861), mimetypes on Windows (CVE-2024-3220),
pyexpat (CVE-2026-4224), bz2 (CVE-2026-9669), ElementPath
(CVE-2026-6879), urllib2 (CVE-2026-15806), HTMLParser (CVE-2026-15308),
bundled Expat 2.9.0 (30 CVEs), the Expat hash salt (CVE-2026-7210), the
ssl SNI callback (CVE-2026-19445), unicodedata (CVE-2026-3276) and
os.mkdir on Windows (CVE-2024-4030), plus the review follow-up build
fixes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants