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

test: Incorrect assumptions on the user uid and gid #19371

Closed
gireeshpunathil opened this issue Mar 15, 2018 · 5 comments
Closed

test: Incorrect assumptions on the user uid and gid #19371

gireeshpunathil opened this issue Mar 15, 2018 · 5 comments
Labels
good first issue Issues that are suitable for first-time contributors. test Issues and PRs related to the tests.

Comments

@gireeshpunathil
Copy link
Member

  • Version: master
  • Platform: UNIX
  • Subsystem: test

The count of expected errors here does not take into consideration of the conditionals here and is assumed to be always true.

This fails always if the user is root or sometimes in containers where process.getuid() and process.getgid() can be 0.

ref: nodejs/help#687

@gireeshpunathil gireeshpunathil added the test Issues and PRs related to the tests. label Mar 15, 2018
@bnoordhuis
Copy link
Member

@gireeshpunathil Links to branches (master) aren't stable. Use links to tags or commits.

Pro tip: if you go to https://github.com/nodejs/node/blob/master/test/parallel/test-child-process-spawnsync-validation-errors.js#L14 and press y, it turns the URL into a commit link.

@gireeshpunathil gireeshpunathil added the good first issue Issues that are suitable for first-time contributors. label Mar 20, 2018
@garwahl
Copy link
Contributor

garwahl commented Mar 22, 2018

@gireeshpunathil How would I begin working on this? Could you elaborate on what needs to be done

@gireeshpunathil
Copy link
Member Author

@garwahl -

this line has the number of expected errors statically determined to be 62. This is based on the assumption of the condition at here and here will be true. When ran as root or ran in certain Containers, this may not be the case.

So:

  1. Count the # of invalidArgTypeError that comes under these two sections separately.
  2. Define 1 variabales, assign 62 to that
  3. if not windows && process.getuid() === 0, reduce the count of invalidArgTypeError coming from that block, from the variable: 10
  4. if not windows && process.getgid() === 0, reduce the count further accordingly.
  5. apply the variable in place of 62.
  6. Add one liner comment against each of your changes so that someone does not stumble on the same issue in future.
  7. test in windows, non-windows, container & non-container environments if possible.

Hope this helps!

@garwahl
Copy link
Contributor

garwahl commented Mar 22, 2018

Thanks, I'll make a start and let you know if I run into any issues.

garwahl added a commit to garwahl/node that referenced this issue Mar 23, 2018
Add a invalidArgTypeErrorCount variable to adjust the number of expected
errors if the uid and gid options cannot be properly validated.

Fixes: nodejs#19371
@garwahl
Copy link
Contributor

garwahl commented Mar 27, 2018

@gireeshpunathil Please review PR when free, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants