Skip to content

Fix: Module error handling, cleanup callback panic handling#48

Merged
lvkv merged 2 commits into
mainfrom
lvkv/fixes-8
May 26, 2026
Merged

Fix: Module error handling, cleanup callback panic handling#48
lvkv merged 2 commits into
mainfrom
lvkv/fixes-8

Conversation

@lvkv
Copy link
Copy Markdown
Owner

@lvkv lvkv commented May 26, 2026

Two module-related fixes:

  1. The current code can technically return Err(PAM_SUCCESS) if the library returns a PAM_SUCCESS code and simultaneously hands back a null pointer (for whatever reason). This would lead to an outcome not unlike Unsound unsafe use in PamHandle::get_user #16 (comment) ) (which looked like: failed to get username, error code: PAM_SUCCESS). This PR updates the code to return an PAM_SYSTEM_ERR in this edge case. Also update the "string isn't valid UTF-8" error code to PAM_SYSTEM_ERR and documents it instead of the previously undocumented PAM_CONV_ERR.
  2. The Rust-written cleanup callback we pass over the FFI boundary to PAM can panic, which is Not Good. This updates the callback (which is otherwise quite simple) to essentially avoid panics at all costs. This is surprisingly difficult to get right (drops can panic... and the thing that catches the panic can panic... and if all else fails, just forget/leak), and I've added some hopefully helpful comments and a test around the actual fix. See:
    a. https://internals.rust-lang.org/t/some-thoughts-on-a-less-slippery-catch-unwind/16902/4
    b. Footgun with catch_unwind when catching panic-on-drop types rust-lang/rust#86027

@lvkv lvkv requested a review from Copilot May 26, 2026 22:05
@lvkv lvkv self-assigned this May 26, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request hardens PAM module FFI interactions by improving edge-case error handling and ensuring Rust panics cannot unwind across the C cleanup callback boundary.

Changes:

  • Prevents get_data / get_user from returning Err(PAM_SUCCESS) by mapping “success + null pointer” to PAM_SYSTEM_ERR.
  • Changes invalid-UTF8 username handling to return PAM_SYSTEM_ERR and documents the behavior.
  • Wraps the pam_set_data cleanup callback in robust panic containment logic and adds a regression test for panicking drops.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lvkv lvkv merged commit 057031f into main May 26, 2026
6 checks passed
@lvkv lvkv deleted the lvkv/fixes-8 branch May 27, 2026 18:58
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