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.

Sign up for GitHub

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

Jump to bottom

Fix #1171, check chmod return #1179

Closed
wants to merge 1 commit into from
Closed

Fix #1171, check chmod return #1179

wants to merge 1 commit into from

Conversation

martintc
Copy link

@martintc martintc commented Oct 8, 2021 •
edited by skliper
Loading

Describe the contribution

Check the return values on the calls to chmod

Testing performed
Using the CI/ID pipeline on github.

Expected behavior changes
No expected behavior change

System(s) tested on

  • OS: Ubuntu 18.04

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Full name and company/organization/center of all contributors ("Personal" if individual work)

  • If NASA Civil Servant Employee or GSFC Contractor on SES II
    • Address/email/phone and contract/task information (if applicable) must be on file
  • Else if Company
  • Else if Individual

JH edit: added fix keyword to autolink

skliper
skliper requested changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Simplification recommendation - use fchmod to avoid time of check/time of use warnings. The stat calls aren't needed, so remove. Really the fchmod is to avoid another warning about creating a file without limiting permissions. Simply call fchmod and if != 0 printf a message about not being able to limit permissions on the filename.

@astrogeco astrogeco changed the title Fix#1171 check chmod return Fix #1171, check chmod return Nov 3, 2021
@astrogeco astrogeco added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 3, 2021
@astrogeco
Copy link
Contributor

Hi! Thanks for your contribution!

Can you apply the standard clang-format to fix the findings in https://github.com/nasa/osal/runs/3883173124?check_suite_focus=true

Also, can you submit a contributor license agreement?

@astrogeco astrogeco added the CCB:PendingCLA External contribution pending CLA confirmation label Nov 3, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Nov 3, 2021 •
edited
Loading

One more thing, can you squash the commits and format the messages to match our standard? Check out https://github.com/nasa/cFS/blob/main/CONTRIBUTING.md#once-a-pull-request-ready-for-review for some guidelines

@astrogeco astrogeco removed the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 3, 2021
@martintc
Fix nasa#1171, check chmod return
4720153 
Squashed commit of the following:

commit b276438
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Thu Nov 4 09:50:31 2021 -0700

    remove white space for clanf-format

commit 491d173
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Thu Nov 4 09:49:04 2021 -0700

    fix indentation

commit 5962ec8
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Thu Nov 4 09:40:50 2021 -0700

    Conform to clang-style

commit ed2c0cb
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Tue Oct 12 06:03:23 2021 -0700

    Fix formatting

commit 99a8536
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Mon Oct 11 08:41:02 2021 -0700

    Get information about file using fstat

    Needed for fchmod.

commit c759f8a
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Mon Oct 11 08:33:56 2021 -0700

    Add in missing parenthesis

commit 220132d
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Mon Oct 11 08:32:38 2021 -0700

    Error handling for unable to limit permissions

commit 249c3f9
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Mon Oct 11 08:30:31 2021 -0700

    Fix formatting

commit 1fdfa64
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Mon Oct 11 08:29:34 2021 -0700

    Use fileno and error handling on unable to limit permissions

commit 810a316
Author: Todd Martin <kj4ntv@gmail.com>
Date:   Mon Oct 11 08:22:21 2021 -0700

    Remove use of fstat and exchanged chmod with fchmod

commit 7a3cf3f
Author: Todd Martin <toddmartin@Todds-MacBook-Air.local>
Date:   Fri Oct 8 00:07:46 2021 -0700

    Fix name of variable during closing of file

commit 3ab23ac
Author: Todd Martin <toddmartin@Todds-MacBook-Air.local>
Date:   Fri Oct 8 00:02:59 2021 -0700

    Fix formatting

commit 355a1dc
Author: Todd Martin <toddmartin@Todds-MacBook-Air.local>
Date:   Fri Oct 8 00:02:43 2021 -0700

    Check the return value of chmod

commit 77db4f4
Author: Todd Martin <toddmartin@Todds-MacBook-Air.local>
Date:   Thu Oct 7 23:58:54 2021 -0700

    Add in stat after chmod

commit 6a63daf
Author: Todd Martin <toddmartin@Todds-MacBook-Air.local>
Date:   Thu Oct 7 23:51:03 2021 -0700

    Add another return false

    This return of false will be hit if all three calls in the nested if statements
    fails. In the case of chmod, stat and fopen failing.

commit d7cbf36
Author: Todd Martin <toddmartin@Todds-MacBook-Air.local>
Date:   Thu Oct 7 23:48:41 2021 -0700

    Check return value of chmod

    Rearranged the source since while checking if equal to zero, the chmod is
    performed along with the stat function. Previous implementation invoked stat
    twice in the same context leading to a redundant action.
@martintc
Copy link
Author

martintc commented Nov 4, 2021

One more thing, can you squash the commits and format the messages to match our standard? Check out https://github.com/nasa/cFS/blob/main/CONTRIBUTING.md#once-a-pull-request-ready-for-review for some guidelines

Formatting should be good to go. Hope I did the squashing of commits correctly. I will also work on getting the contributing license agreement in

@martintc
Copy link
Author

martintc commented Nov 8, 2021

As request, individual CLA is signed and sent to the email address provided on the form.

@astrogeco astrogeco added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) and removed CCB:PendingCLA External contribution pending CLA confirmation CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Dec 1, 2021
@astrogeco
Copy link
Contributor

CCB:2021-12-08 - REVIEW AGAIN

  • Need to look into this and ensure it works with C99 and not posix

@skliper skliper linked an issue Dec 8, 2021 that may be closed by this pull request
Check Return Value of chmod #1171
Open
@skliper
Copy link
Contributor

skliper commented Dec 8, 2021

My mistake, I shouldn't have recommended using a POSIX function in utassert. Will need to refactor slightly.

@skliper
Copy link
Contributor

skliper commented Dec 9, 2021

Adding the stat and chmod (original line 64-67) stems from trying to squash a "File created without restricting permissions" alert, and since there's no great way I'm aware of to do that w/ just C99 without introducing a TOCTOU alert... and even with POSIX dependency it still throws the original alert I think we should take a step back and remove the original change, accept that the user needs to have reasonable default permissions, and add a comment detailing our approach/disposition of the alert. We really can't correct for the alert in a good way whenever creating files within the software.

@astrogeco astrogeco force-pushed the integration-candidate branch 3 times, most recently from c9ca9ab to 1d591d8 Compare March 16, 2022 21:46
@astrogeco astrogeco added CCB:Ignore Incomplete Pull Request with open actions. wontfix and removed CCB:Ignore Incomplete Pull Request with open actions. wontfix labels Jun 10, 2022
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
@astrogeco
Merge pull request nasa#1179 from zanzaben/fix1149_Remove_cfeesugshel…
454f035 
…lsrv_guide

Fix nasa#1149, remove cfeesugshellsrv from user guide
@dzbaker
Copy link
Collaborator

dzbaker commented Sep 8, 2022

CCB 8 September 2022: May just want to open issue to go back to old implementation and close this one.

@dzbaker dzbaker self-assigned this Sep 8, 2022
@dzbaker dzbaker added the CCB:Ignore Incomplete Pull Request with open actions. label Sep 8, 2022
@dzbaker dzbaker mentioned this pull request Sep 8, 2022
Merged
2 tasks
@dzbaker dzbaker closed this Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skliper skliper Awaiting requested review from skliper

@zanzaben zanzaben Awaiting requested review from zanzaben

@jphickey jphickey Awaiting requested review from jphickey

Assignees

@dzbaker dzbaker

Labels
CCB:Ignore Incomplete Pull Request with open actions. community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check Return Value of chmod
4 participants
@martintc @astrogeco @skliper @dzbaker