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

Check Return Value of chmod #1171

Open
ArielSAdamsNASA opened this issue Sep 29, 2021 · 2 comments
Open

Check Return Value of chmod #1171

ArielSAdamsNASA opened this issue Sep 29, 2021 · 2 comments
Assignees
Labels
bug good first issue Good for newcomers

Comments

@ArielSAdamsNASA
Copy link
Contributor

ArielSAdamsNASA commented Sep 29, 2021

Describe the bug
Calling chmod(Filename, dststat.st_mode & 0xffffffc0U) without checking return value. This library function may fail and return an error code.

Expected behavior
Check the return value of chmod.

Code snips

chmod(Filename, dststat.st_mode & ~(S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH));

chmod(Filename, dststat.st_mode & ~(S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH));

Additional context
From Coverity: https://scan.coverity.com/projects/arielsadamsnasa-cfs-jsf-rules?tab=overview

Possible solution:

if (chmod(Filename, dststat.st_mode & ~(S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH)) == 0) 
{
      chmod(Filename, dststat.st_mode & ~(S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH));
      stat(Filename, &dststat);
}

Reporter Info
Ariel Adams, ASRC Federal

@martintc
Copy link

martintc commented Oct 7, 2021

I can take this issue.

@skliper
Copy link
Contributor

skliper commented Oct 8, 2021

Recommendation - use fchmod to avoid time of check/time of use warnings. The stat calls aren't needed, so remove. Really the chmod 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.

martintc added a commit to martintc/osal that referenced this issue Nov 4, 2021
Squashed commit of the following:

commit b276438
Author: Todd Martin <[email protected]>
Date:   Thu Nov 4 09:50:31 2021 -0700

    remove white space for clanf-format

commit 491d173
Author: Todd Martin <[email protected]>
Date:   Thu Nov 4 09:49:04 2021 -0700

    fix indentation

commit 5962ec8
Author: Todd Martin <[email protected]>
Date:   Thu Nov 4 09:40:50 2021 -0700

    Conform to clang-style

commit ed2c0cb
Author: Todd Martin <[email protected]>
Date:   Tue Oct 12 06:03:23 2021 -0700

    Fix formatting

commit 99a8536
Author: Todd Martin <[email protected]>
Date:   Mon Oct 11 08:41:02 2021 -0700

    Get information about file using fstat

    Needed for fchmod.

commit c759f8a
Author: Todd Martin <[email protected]>
Date:   Mon Oct 11 08:33:56 2021 -0700

    Add in missing parenthesis

commit 220132d
Author: Todd Martin <[email protected]>
Date:   Mon Oct 11 08:32:38 2021 -0700

    Error handling for unable to limit permissions

commit 249c3f9
Author: Todd Martin <[email protected]>
Date:   Mon Oct 11 08:30:31 2021 -0700

    Fix formatting

commit 1fdfa64
Author: Todd Martin <[email protected]>
Date:   Mon Oct 11 08:29:34 2021 -0700

    Use fileno and error handling on unable to limit permissions

commit 810a316
Author: Todd Martin <[email protected]>
Date:   Mon Oct 11 08:22:21 2021 -0700

    Remove use of fstat and exchanged chmod with fchmod

commit 7a3cf3f
Author: Todd Martin <[email protected]>
Date:   Fri Oct 8 00:07:46 2021 -0700

    Fix name of variable during closing of file

commit 3ab23ac
Author: Todd Martin <[email protected]>
Date:   Fri Oct 8 00:02:59 2021 -0700

    Fix formatting

commit 355a1dc
Author: Todd Martin <[email protected]>
Date:   Fri Oct 8 00:02:43 2021 -0700

    Check the return value of chmod

commit 77db4f4
Author: Todd Martin <[email protected]>
Date:   Thu Oct 7 23:58:54 2021 -0700

    Add in stat after chmod

commit 6a63daf
Author: Todd Martin <[email protected]>
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 <[email protected]>
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.
@skliper skliper linked a pull request Dec 8, 2021 that will close this issue
jphickey pushed a commit to jphickey/osal that referenced this issue Aug 10, 2022
cFE Integration candidate: 2021-02-23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants