Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose PamEnvList and PamEnvIter API #15

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

regiontog
Copy link
Contributor

No description provided.

Copy link
Owner

@1wilkens 1wilkens left a comment

Choose a reason for hiding this comment

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

Made some small comments but overall this looks good!

src/env.rs Outdated Show resolved Hide resolved
src/env.rs Outdated Show resolved Hide resolved
src/env.rs Outdated Show resolved Hide resolved
@1wilkens
Copy link
Owner

I merged your other PR first, so this needs a rebase, but after that I'll merge this.
If you could also write a CHANGELOG entry, that would be awesome, if not I'll do it after merging ;)

@regiontog
Copy link
Contributor Author

regiontog commented May 29, 2019

I tested the null check on the first indirection and I unfortunately the assertion failed immediately.

thread 'main' panicked at 'assertion failed: unsafe { !(*env_ptr).is_null() }', /home/alan/dev/regiontog/pam/src/env.rs:52:13

I know it worked with the check on only the second indirection, but I don't understand why.
I logged the pointers as the iterator was ran and it seems like the variables stop after the inner pointer is null:

env ptr: 0x55eded603450
deref env ptr: 0x55eded604f40
env ptr: 0x55eded603458
deref env ptr: 0x55eded604e70
env ptr: 0x55eded603460
deref env ptr: 0x55eded606590
env ptr: 0x55eded603468
deref env ptr: 0x55eded6049e0
env ptr: 0x55eded603470
deref env ptr: 0x55eded603550
env ptr: 0x55eded603478
deref env ptr: 0x55eded603520
env ptr: 0x55eded603480
deref env ptr: 0x55eded6034f0
env ptr: 0x55eded603488
deref env ptr: 0x55eded600010
env ptr: 0x55eded603490
deref env ptr: 0x55eded6034c0
env ptr: 0x55eded603498
deref env ptr: 0x55eded603390
env ptr: 0x55eded6034a0
deref env ptr: 0x55eded603300
env ptr: 0x55eded6034a8
deref env ptr: 0x55eded6053e0
env ptr: 0x55eded6034b0
deref env ptr: 0x0
env ptr: 0x55eded6034b8
deref env ptr: 0x31
segmentation fault (core dumped)

@1wilkens
Copy link
Owner

That's interesting which test did you run? I tried the spawn_bash example but on my machine I never got any valid pointers on the second level even for the first call to iter which immediately segfaulted..

Anyway your results then indicate, that we just check both for null and stop the iterator once that doesn't hold anymore.

@regiontog
Copy link
Contributor Author

regiontog commented May 29, 2019

That's interesting which test did you run?

I just ran my DM, I did not check if too closely but I assume it was the initialize_environment called from open_session that ran the iterator to a segfault.

@1wilkens
Copy link
Owner

1wilkens commented Jun 3, 2019

Could you rebase this once more on master and I'll merge it. Sorry for the delay :)

@1wilkens
Copy link
Owner

Sorry for the delay. It seems the commit history in this PR is messed up after the merges (duplicate commits). I tried to squash some commits but failed (at least in short time).
Either you start from the current master and try to integrate your changes or I will do so in some time as I would still like to have the API. Anyway, I am working on some more refactoring on the branch pam-sys-next so you might want to wait until this hits master anyways. Again sorry for the inconvenience!

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