Fix nil error issue #243

bitcodr · 2020-07-31T15:03:17Z

Hi,
I've fixed a bug related issue #242,

google-cla · 2020-07-31T15:03:22Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-07-31T15:06:20Z

@googlebot I signed it!

@googlebot I signed it!

google-cla · 2020-07-31T15:06:24Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-07-31T15:11:39Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

information_source Googlers: Go here for more info.

@googlebot I fixed it.

google-cla · 2020-07-31T15:11:43Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-08-01T13:04:15Z

@josephlr Could you please review my first pull request in the google open source community?

ebiggers

I'm not sure this is a good idea. newExitError() should only be called if there was actually an error. Making it silently ignore nil errors could cause the program report success when actually something failed.

Can you fix the caller instead?

#242 says the problem is:

    // Sanity check before unlocking everything
    if err := policy.AddProtector(protector); errors.Cause(err) != actions.ErrLocked {
            return newExitError(c, err)
    }

It can be changed to:

    if err := policy.AddProtector(protector); errors.Cause(err) != actions.ErrLocked {
            if err == nil {
                    // should never happen
                    err = errors.New("policy and protector are not locked!")
            }
            return newExitError(c, err)
    }

I don't see how the err == nil case can actually happen, though. It should be impossible. Do you have a reproducer?

google-cla · 2020-08-01T18:14:57Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-08-01T18:17:04Z

I'm not sure this is a good idea. newExitError() should only be called if there was actually an error. Making it silently ignore nil errors could cause the program report success when actually something failed.

Can you fix the caller instead?

#242 says the problem is:
    // Sanity check before unlocking everything
    if err := policy.AddProtector(protector); errors.Cause(err) != actions.ErrLocked {
            return newExitError(c, err)
    }
It can be changed to:
    if err := policy.AddProtector(protector); errors.Cause(err) != actions.ErrLocked {
            if err == nil {
                    // should never happen
                    err = errors.New("policy and protector are not locked!")
            }
            return newExitError(c, err)
    }
I don't see how the err == nil case can actually happen, though. It should be impossible. Do you have a reproducer?

Thank you for review @ebiggers
What if adding a nil error statement before the errors.Cause(err) as below

 if err := policy.AddProtector(protector); err != nil && errors.Cause(err) != actions.ErrLocked {
            return newExitError(c, err)
    }

ebiggers · 2020-08-01T18:22:05Z

What if adding a nil error statement before the errors.Cause(err) as below
 if err := policy.AddProtector(protector); err != nil && errors.Cause(err) != actions.ErrLocked {
            return newExitError(c, err)
    }

No, that's not right. We expect AddProtector() to return actions.ErrLocked. If it doesn't, we want to return with an error (thus causing fscrypt add-protector-to-policy to fail rather than continue on).

google-cla · 2020-08-02T10:17:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-08-02T10:20:19Z

What if adding a nil error statement before the errors.Cause(err) as below
 if err := policy.AddProtector(protector); err != nil && errors.Cause(err) != actions.ErrLocked {
            return newExitError(c, err)
    }
No, that's not right. We expect AddProtector() to return actions.ErrLocked. If it doesn't, we want to return with an error (thus causing fscrypt add-protector-to-policy to fail rather than continue on).

You're right, I changed it

ebiggers · 2020-08-05T01:57:37Z

Can you please run make format and squash your commits?

google-cla · 2020-08-05T06:42:50Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-05T06:56:23Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-05T06:59:47Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-05T07:03:31Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-08-05T07:04:33Z

Can you please run make format and squash your commits?

Yes, I've done

ebiggers · 2020-08-05T07:08:33Z

Can you fix the presubmit failures?

CLA not signed, or commit author is wrong
make lint fails

bitcodr · 2020-08-05T20:06:53Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

information_source Googlers: Go here for more info.

@googlebot I fixed it.

google-cla · 2020-08-05T20:06:56Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-06T05:38:45Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-08-06T05:39:43Z

@googlebot I fixed it.

@googlebot I fixed it.

google-cla · 2020-08-06T05:39:46Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

bitcodr · 2020-08-06T05:42:12Z

@googlebot I fixed it.

@googlebot I fixed it.

bitcodr · 2020-08-06T06:18:06Z

Can you fix the presubmit failures?
* CLA not signed, or commit author is wrong

* `make lint` fails

CLA signed, but the lint errors are:

go vet ./...
# github.com/google/fscrypt/pam
pam/constants.go:51:10: fatal error: security/pam_modules.h: No such file or directory
   51 | #include <security/pam_modules.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
# github.com/google/fscrypt/pam
pam/constants.go:51:10: fatal error: security/pam_modules.h: No such file or directory
   51 | #include <security/pam_modules.h>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:109: lint] Error 2

ebiggers · 2020-08-06T06:25:32Z

You need to install the PAM development package for your Linux distro. See https://github.com/google/fscrypt#building-and-installing

bitcodr · 2020-08-07T06:17:13Z

You need to install the PAM development package for your Linux distro. See https://github.com/google/fscrypt#building-and-installing

Thank you @ebiggers,
Coverage failed but there is no test file for it.!!

ebiggers · 2020-08-07T23:31:38Z

Great, we're almost there. (The coverage test doesn't need to pass.)

Can you please add the issue number to the commit message:

Resolves https://github.com/google/fscrypt/issues/242

bitcodr · 2020-08-08T05:24:48Z

Great, we're almost there. (The coverage test doesn't need to pass.)

Can you please add the issue number to the commit message:
Resolves https://github.com/google/fscrypt/issues/242

Done.

ebiggers · 2020-08-09T16:17:22Z

Merged, thanks.

For future reference, Resolves should go on a separate line.

bitcodr marked this pull request as draft July 31, 2020 15:12

bitcodr marked this pull request as ready for review July 31, 2020 15:13

ebiggers requested changes Aug 1, 2020

View reviewed changes

bitcodr requested a review from ebiggers August 5, 2020 07:04

Fix nil error issue, Resolves #242

9077008

ebiggers merged commit 5e85ae0 into google:master Aug 9, 2020

bitcodr deleted the fix-nil-error-bug branch August 9, 2020 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nil error issue #243

Fix nil error issue #243

bitcodr commented Jul 31, 2020

google-cla bot commented Jul 31, 2020

bitcodr commented Jul 31, 2020

google-cla bot commented Jul 31, 2020

bitcodr commented Jul 31, 2020

google-cla bot commented Jul 31, 2020

bitcodr commented Aug 1, 2020

ebiggers left a comment

google-cla bot commented Aug 1, 2020

bitcodr commented Aug 1, 2020

ebiggers commented Aug 1, 2020

google-cla bot commented Aug 2, 2020

bitcodr commented Aug 2, 2020

ebiggers commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

bitcodr commented Aug 5, 2020

ebiggers commented Aug 5, 2020

bitcodr commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 6, 2020

bitcodr commented Aug 6, 2020

google-cla bot commented Aug 6, 2020

bitcodr commented Aug 6, 2020

bitcodr commented Aug 6, 2020

ebiggers commented Aug 6, 2020

bitcodr commented Aug 7, 2020

ebiggers commented Aug 7, 2020

bitcodr commented Aug 8, 2020

ebiggers commented Aug 9, 2020

Fix nil error issue #243

Fix nil error issue #243

Conversation

bitcodr commented Jul 31, 2020

google-cla bot commented Jul 31, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

bitcodr commented Jul 31, 2020

google-cla bot commented Jul 31, 2020

bitcodr commented Jul 31, 2020

google-cla bot commented Jul 31, 2020

bitcodr commented Aug 1, 2020

ebiggers left a comment

Choose a reason for hiding this comment

google-cla bot commented Aug 1, 2020

bitcodr commented Aug 1, 2020

ebiggers commented Aug 1, 2020

google-cla bot commented Aug 2, 2020

bitcodr commented Aug 2, 2020

ebiggers commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

bitcodr commented Aug 5, 2020

ebiggers commented Aug 5, 2020

bitcodr commented Aug 5, 2020

google-cla bot commented Aug 5, 2020

google-cla bot commented Aug 6, 2020

bitcodr commented Aug 6, 2020

google-cla bot commented Aug 6, 2020

bitcodr commented Aug 6, 2020

bitcodr commented Aug 6, 2020

ebiggers commented Aug 6, 2020

bitcodr commented Aug 7, 2020

ebiggers commented Aug 7, 2020

bitcodr commented Aug 8, 2020

ebiggers commented Aug 9, 2020