Fix: initialize MData and AXUIElementRef to avoid segfault in get_active

[PekingSpades]

Summary

Fix segmentation fault (SIGSEGV) in get_active() and GetTitle() functions caused by uninitialized variables, particularly in headless CI environments.

Fixes: #754

Problem

The get_active() function in window/window.h has uninitialized local variables that cause undefined behavior:

MData get_active(void) {
    MData result;              // Uninitialized - contains garbage values
    // ...
    AXUIElementRef element;    // Uninitialized - contains garbage pointer
    // ...
    if (AXUIElementCopyAttributeValue(...) == kAXErrorSuccess && element) {
        // success path
    } else {
        result.AxID = element; // Assigns garbage value to result!
    }
    return result;
}

When AXUIElementCopyAttributeValue() fails (common in CI environments without accessibility permissions), the uninitialized element variable contains a garbage pointer value. This garbage value is then used in subsequent AXUIElement* calls, causing SIGSEGV.

CI Environment Specifics

GitHub Actions macOS runners cannot grant Accessibility Permissions because System Integrity Protection (SIP) is enabled. This causes AXUIElementCopyAttributeValue() to fail, triggering the buggy code path.

Root Cause Analysis

According to SEI CERT C Coding Standard (EXP33-C):

"Reading uninitialized variables is undefined behavior and can result in unexpected program behavior. In some cases, these security flaws may allow the execution of arbitrary code."

The C Standard specifies:

"If an object that has automatic storage duration is not initialized explicitly, its representation is indeterminate."

Effect

Scenario	Before	After
CI (no accessibility permission)	SIGSEGV crash	Returns safely with is_valid() = false
Normal environment	Works (by luck)	Works (guaranteed)

Testing

• Fixes CI test failure in TestGetTitle
• All platforms (macOS, Windows, X11) initialized consistently

References

• SEI CERT C Coding Standard - EXP33-C: Do not read uninitialized memory
• GitHub Actions runner-images #3286 - Accessibility Permissions
• Learn C++ - Uninitialized variables and undefined behavior
• Apple Developer Documentation - AXUIElementCopyAttributeValue

Summary by CodeRabbit

• Bug Fixes
  • Fixed potential stability issues related to uninitialized data in window detection across macOS, X11, and Windows platforms.
  • Improved error handling to ensure window information is only populated when successfully retrieved.
  • Enhanced resource management and control flow for more reliable window operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request addresses a SIGSEGV crash in the get_active() function on macOS by initializing previously uninitialized local variables (MData result and AXUIElementRef element) to zero/null values. The fix removes logic that assigned uninitialized values in error paths and ensures result is only populated on successful attribute retrieval.

Changes

Cohort / File(s)	Summary
Initialization and Control Flow window/window.h	Initialize MData result structure to zero on macOS; set AXUIElementRef element to NULL before use; remove code path that assigned uninitialized element to result.AxID on attribute retrieval failure; apply zero-initialization pattern for result in X11 and Windows paths to prevent use of uninitialized data

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

bug

Suggested reviewers

• vcaesar

Poem

🐰 A crash in the code made the CI weep,
With pointers uninitialized, jumping so deep,
But now they're set NULL, with zeros in place,
No more SIGSEGV crashes to face!
The rabbit hops gladly, the macOS runs clean,
The finest bug fix the CI's ever seen! 🎉

Pre-merge checks and finishing touches

❌ 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%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main fix: initializing MData and AXUIElementRef to prevent segmentation faults in get_active.
Description check	✅ Passed	The description comprehensively addresses all requirements: explains the problem with code examples, provides root cause analysis with standards references, links to issue #754, and documents testing across all platforms.
Linked Issues check	✅ Passed	The PR directly addresses issue #754 by initializing MData result and AXUIElementRef element to prevent garbage pointer dereference and SIGSEGV crashes in CI environments.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing uninitialized variables in window/window.h across macOS, Windows, and X11 paths, directly addressing the linked issue #754 with no extraneous modifications.

✨ Finishing touches

• 📝 Generate docstrings

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a38884 and 6a647b5.

📒 Files selected for processing (1)

• window/window.h

🔇 Additional comments (1)

window/window.h (1)

373-384: LGTM: Correct logic and memory management.

The refactored logic correctly handles all cases:

1. Success path: Both conditions succeed → result is populated with valid values, element ownership transfers to caller (no release here).
2. Partial failure: Outer condition succeeds but inner fails → element is properly released (line 382), result remains zero-initialized.
3. Early failure: Outer condition fails → element remains NULL, no release needed, result remains zero-initialized.

Memory management is sound. The caller is responsible for releasing result.AxID (handled via pub_mData.AxID cleanup in setHandle() at lines 179-181).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical fix: zero-initialize result to prevent undefined behavior.

This initialization is essential to ensure that all error paths return deterministic zero values instead of garbage memory. Without this, callers like is_valid() could dereference invalid pointers when checking result.AxID, causing the SIGSEGV described in issue #754.

🤖 Prompt for AI Agents

In window/window.h around line 353, the MData variable 'result' is not being
zero-initialized which can leave fields (like result.AxID) containing garbage
and cause undefined behavior or SIGSEGV on error paths; fix this by initializing
result to zero (e.g., set all bytes/fields to zero) at declaration so all
callers receive deterministic zeroed fields on failure and avoid dereferencing
invalid pointers.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Critical fix: initialize element to NULL.

Initializing element to NULL prevents garbage pointer propagation. Previously, if AXUIElementCopyAttributeValue() failed (common in CI environments without accessibility permissions), the uninitialized element could contain a garbage pointer that might pass the null check and be assigned to result.AxID, leading to crashes when dereferenced downstream.

🤖 Prompt for AI Agents

In window/window.h around line 370, the local variable 'element' must be
explicitly initialized to NULL to avoid propagation of a garbage pointer when
AXUIElementCopyAttributeValue() fails; update the declaration so 'element' is
initialized to NULL, ensuring subsequent null checks are reliable and preventing
invalid pointers from being assigned to result.AxID.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Good: Consistent zero-initialization across all platforms.

Applying the same initialization pattern to X11 and Windows paths ensures consistent, deterministic behavior across all platforms and eliminates undefined behavior. This aligns with secure coding best practices (CERT C EXP33-C) and prevents similar issues from manifesting on other platforms.

Also applies to: 428-428

🤖 Prompt for AI Agents

In window/window.h around lines 389 and 428, the MData structures are not
explicitly zero-initialized on both X11 and Windows code paths; change the
declarations to use explicit zero-initialization (e.g., MData result = {0};) at
both locations so the structs are deterministically initialized across platforms
and avoid undefined behavior.