Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix Issue 21789: Let umask prune permissions of coverage lst files #3421

Merged
merged 1 commit into from Apr 3, 2021

Conversation

omerfirmak
Copy link
Contributor

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Apr 1, 2021

Thanks for your pull request and interest in making D better, @omerfirmak! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21789 enhancement Codecov should use default umask for file permissions

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3421"

@thewilsonator
Copy link
Contributor

Please update the commit message to say "Fix issue 21789: ..."

@omerfirmak
Copy link
Contributor Author

Please update the commit message to say "Fix issue 21789: ..."

Done

@thewilsonator
Copy link
Contributor

Its hard to understand from the issue what the problem is . Also this should have a test case and target master

@omerfirmak
Copy link
Contributor Author

Its hard to understand from the issue what the problem is .

Which part exactly? I think the problem is pretty self-explanatory.

Also this should have a test case

I can't think of a sane way to test this.

and target master

sigh

src/rt/cover.d Show resolved Hide resolved
@ibuclaw
Copy link
Member

ibuclaw commented Apr 2, 2021

Its hard to understand from the issue what the problem is .

Which part exactly? I think the problem is pretty self-explanatory.

Also this should have a test case

I can't think of a sane way to test this.

Get the umask in effect and check the output file matches what is expected? It can be done in a shell script. I'm sure there are examples of shell tests in the druntime testsuite.

@omerfirmak
Copy link
Contributor Author

Its hard to understand from the issue what the problem is .

Which part exactly? I think the problem is pretty self-explanatory.

Also this should have a test case

I can't think of a sane way to test this.

Get the umask in effect and check the output file matches what is expected? It can be done in a shell script. I'm sure there are examples of shell tests in the druntime testsuite.

So you want me to test the open syscall?

@Geod24
Copy link
Member

Geod24 commented Apr 2, 2021

To add a bit on this issue:

  1. Why are the coverage at mode 600 to begin with ?
    The only thing I can think of is to "protect" the source code from accidental leaks, but for it to be a useful protection, the attacker would need a level of access which implies a lot of other, easier attack vectors.
    For example for coverage to work, the source code needs to be accessible, so if one can access the directory in which the .lst files are located, one is also likely to have access to the source files, which are unlikely to be 600.

  2. Why was this a problem ?
    Our program runs its integration tests inside of Docker, and creates those files as root, which means that later on the coverage upload fails (we use Github CI). There are workarounds for this, but it would propagate everywhere, for a problem which shouldn't be there in the first place. One of the workaround is to pass arguments to Docker (--user), but that falls over as soon as you build a more complex pipeline (we spawn a full network using docker-compose, not calling docker directly). The workaround we came up with was to sudo chmod those files, and we actually have to do that in a loop because the list of arguments is so long it reaches the shell's limit. And again, why are we not following umask to begin with ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Enhancement New functionality
Projects
None yet
5 participants