Skip to content

Fix crash on connection attribute lookup with non-string names - #810

Merged
methane merged 1 commit into
mainfrom
fix-809-connection-getattribute
Oct 30, 2026
Merged

methane merged 1 commit into
mainfrom
fix-809-connection-getattribute

Conversation

@methane

@methane methane commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Calling Connection.__new__(Connection).__getattribute__(5) segfaults because PyUnicode_AsUTF8() returns NULL and the attribute getter passes it to strcmp().

Check that the attribute name is Unicode and compare it directly with "closed" using PyUnicode_CompareWithASCIIString(). Other names fall through to PyObject_GenericGetAttr(), which raises the normal TypeError for non-string names. This also handles lone surrogates without UTF-8 conversion and avoids treating "closed\0suffix" as "closed".

Add database-independent regression tests for both _mysql.connection and Connection, covering non-string names, missing names, embedded NULs, lone surrogates, string subclasses, and normal attribute lookup.

Fixes #809.

Validation:

  • Reproduced the original segfault on Python 3.13.15 (exit status 139).
  • Python 3.13.15 / MariaDB 11.8.6: pytest --cov=MySQLdb tests -q — 161 passed.
  • ruff check src/ tests/test__mysql.py and ruff format --check tests/test__mysql.py passed.
  • Ran repository-wide Ruff formatting/checking; unrelated existing formatting changes were reverted, and existing lint findings in CI/other test files remain outside this PR.

Summary by CodeRabbit

  • Bug Fixes
    • Improved attribute access on MySQL connections. The closed attribute can be accessed with regular strings and str subclasses, while invalid or non-string attribute names now produce the expected exceptions. This makes connection attribute lookups more consistent.

@coderabbitai

coderabbitai Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 16caab14-cbde-47e8-a740-da0f052ea185
Commits

Reviewing files that changed from the base of the PR and between ff9e96d and 74b6df0.

Files selected for processing: 2
  • src/MySQLdb/_mysql.c
  • tests/test__mysql.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

Walkthrough

The connection attribute handler now checks that attribute names are Unicode before comparing them with "closed". Tests cover invalid name types, missing and invalid names, and access through both connection classes.

Changes: Connection Attribute Name Handling

Layer / File(s) Summary
Unicode name handling and tests
src/MySQLdb/_mysql.c, tests/test__mysql.py
The handler checks that an attribute name is Unicode before comparing it with "closed". Tests cover invalid name types, missing and invalid names, and access to "closed" through both connection classes.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: Minimal · up to 74b6d

No actionable merge-blocking risk remains in the reviewed changes.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed 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 clearly and concisely describes the main change: preventing crashes during connection attribute lookup with non-string names.
Linked Issues check Passed Issue [#809] requires that non-string names no longer crash connection attribute lookup. The reviewed source uses a Unicode check, compares closed with PyUnicode_CompareWithASCIIString(), and dele…
Out of Scope Changes check Passed The source change directly fixes the attribute lookup path from issue [#809]. The added tests exercise the affected behavior on both connection types. The reviewed changes show no unrelated product be…
Full details: Linked Issues check

Explanation

Issue [#809] requires that non-string names no longer crash connection attribute lookup. The reviewed source uses a Unicode check, compares closed with PyUnicode_CompareWithASCIIString(), and delegates other names to PyObject_GenericGetAttr(). The reviewed tests call __getattribute__ on both _mysql.connection and Connection with non-string names and require TypeError. They also cover missing names, embedded NULs, lone surrogates, string subclasses, and normal closed lookup.

Full details: Out of Scope Changes check

Explanation

The source change directly fixes the attribute lookup path from issue [#809]. The added tests exercise the affected behavior on both connection types. The reviewed changes show no unrelated product behavior or unrelated test changes.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@methane
methane merged commit 2d99fe2 into main Oct 30, 2026
13 checks passed
@methane
methane deleted the fix-809-connection-getattribute branch October 30, 2026 07:41
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.

Calling __getattribute__ with a non-string name on a connection segfaults

2 participants