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

cptbox: don't allow paths that don't exist #1010

Merged
merged 3 commits into from Mar 11, 2022
Merged

Conversation

Riolku
Copy link
Contributor

@Riolku Riolku commented Mar 5, 2022

This is mostly for compatibility with landlock, which enforces this. Builds on top of #999.

Basically, instead of checking that rules are correctly written when 
they are compiled into the jail, we should just check at construction 
time.
This is mostly for compatability with landlock, which enforces this.
@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2022

Codecov Report

Merging #1010 (df90c9e) into master (1b0d26e) will increase coverage by 3.01%.
The diff coverage is 97.82%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1010      +/-   ##
==========================================
+ Coverage   81.06%   84.08%   +3.01%     
==========================================
  Files         140      140              
  Lines        4754     4775      +21     
==========================================
+ Hits         3854     4015     +161     
+ Misses        900      760     -140     
Impacted Files Coverage Δ
dmoj/executors/ZIG.py 100.00% <ø> (ø)
dmoj/cptbox/filesystem_policies.py 97.82% <96.77%> (-0.97%) ⬇️
dmoj/executors/SWIFT.py 100.00% <100.00%> (ø)
dmoj/executors/compiled_executor.py 93.54% <100.00%> (+0.38%) ⬆️
dmoj/tests/test_filesystem_policy.py 100.00% <100.00%> (ø)
dmoj/executors/TEXT.py 100.00% <0.00%> (ø)
dmoj/executors/mixins.py 100.00% <0.00%> (ø)
dmoj/judge.py 52.83% <0.00%> (+1.25%) ⬆️
dmoj/executors/java_executor.py 85.35% <0.00%> (+2.02%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b0d26e...df90c9e. Read the comment docs.

@Riolku Riolku marked this pull request as ready for review March 5, 2022 02:09
@Xyene
Copy link
Member

Xyene commented Mar 5, 2022

Are you sure Rust doesn't need a cache directory? What does it do when it doesn't have one?

@Riolku
Copy link
Contributor Author

Riolku commented Mar 5, 2022

Do you mean ~/.cargo? I'm not sure. I wonder if that's created on Rust installation or something. I'll add it to the list anyway.

@Xyene
Copy link
Member

Xyene commented Mar 5, 2022

I don't think so, but it's worth checking? I know the first compile during autoconf takes a long time, because it has to download a bunch of crates that then get cached.

@Riolku
Copy link
Contributor Author

Riolku commented Mar 5, 2022

RUST passes in CI under this patch. I'm not really sure what it's doing though, and I was having trouble testing this locally. I think docker is re-using my old volumes, or something. Do you have any tips for how I should go about testing this?

@Xyene
Copy link
Member

Xyene commented Mar 5, 2022

Try running docker prune -a then rerunning make judge-tier${whatevertierrustisin}.

Some compilers require directories to write to, specifically, ZIG and 
SWIFT require `.cache`
@Riolku
Copy link
Contributor Author

Riolku commented Mar 5, 2022

Rust actually has .cargo setup in Docker container initialization, so we're ok.

@Riolku
Copy link
Contributor Author

Riolku commented Mar 5, 2022

Actually, if we aren't in Docker, we should create .cargo right? Is it worth adding then?

@Xyene
Copy link
Member

Xyene commented Mar 5, 2022

I don't want to particularly support running outside of Docker, it's too much of a hassle. Users are on their own if they do so anyway.

@Xyene
Copy link
Member

Xyene commented Mar 5, 2022

Specifically: Cargo installs to .cargo, so if they don't have .cargo they probably don't have Cargo installed (or have it at some weird path). Not our problem in either case.

Copy link
Member

@Xyene Xyene left a comment

Choose a reason for hiding this comment

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

LGTM

@Xyene Xyene merged commit 6cd70da into DMOJ:master Mar 11, 2022
@Riolku Riolku deleted the check-exists-path branch March 11, 2022 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants