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

Don't Merge - LTP: Fixed rename09 testcase #56

Open
wants to merge 2 commits into
base: sgx-lkl
Choose a base branch
from

Conversation

vamsikrishna935
Copy link

  • Patch Description:
    Test was failing due to lackof fork support to create child processes.
    So modified the tests using threads instead of fork.
    Please refer test description above

@vamsikrishna935 vamsikrishna935 changed the title Reanme09 LTP: Fixed reanme09 testcase Aug 7, 2020
@vamsikrishna935 vamsikrishna935 changed the title LTP: Fixed reanme09 testcase LTP: Fixed rename09 testcase Aug 7, 2020
Copy link

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

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

I don't understand this change. The original version was creating a child process so that we could do things as nobody and then as root. The modified version appears to be doing setreuid in two different threads, yet the second one should fail once the current real UID is no longer 0, so I don't see how this can possibly pass (the saved UID feature should allow setting UID back to 0, but you should need to do that explicitly).

The threads are not needed at all and complicate this test unnecessarily. Only one is running at any given time.

@hukoyu
Copy link
Collaborator

hukoyu commented Aug 12, 2020

@vamsikrishna935 can you address David's feedback please.

@vamsikrishna935
Copy link
Author

I don't understand this change. The original version was creating a child process so that we could do things as nobody and then as root. The modified version appears to be doing setreuid in two different threads, yet the second one should fail once the current real UID is no longer 0, so I don't see how this can possibly pass (the saved UID feature should allow setting UID back to 0, but you should need to do that explicitly).

The threads are not needed at all and complicate this test unnecessarily. Only one is running at any given time.

@davidchisnall In the original test, it is creating a child process so that we could do things as nobody and then as bin not as root at end it is setting back to root. I tried implementing this without threads but after setting it nobody i am not able set back the UID. With this threads approach i could able to perform.

@hukoyu
Copy link
Collaborator

hukoyu commented Aug 12, 2020

@davidchisnall can we proceed with this multithread approach or close this PR and keep test disabled as non-supported?

@davidchisnall
Copy link

The fact that the modified version is passing appears to indicate a bug to me. I hope @vtikoo's patch will make it fail, so please don't merge it. Does the modified version pass on normal Linux? I don't believe you should be able to change uid on threads independently.

@hukoyu hukoyu changed the title LTP: Fixed rename09 testcase Don't Merge - LTP: Fixed rename09 testcase Aug 13, 2020
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.

3 participants