ioi / isolate

Sandbox for securely executing untrusted programs
Other
1.1k stars 161 forks source link

Configurable open file descriptors limit #85

Closed brainoutsource closed 4 years ago

brainoutsource commented 4 years ago

Greetings! Default ulimit -n set to 64 may not be enough, please consider my attempt at making it configurable. Thanks!

zopieux commented 4 years ago

Thanks, can you fix the whitespaces please? I can see a bunch of them in the GitHub web diff.

brainoutsource commented 4 years ago

@Zopieux I've fixed the whitespaces in the switch statement. The one with the options looks weird in diff but seems to be fine in the raw (there's a double \t there). Please let me know if I'm missing something else.

gollux commented 4 years ago

Before we get to details, I would like to ask for opinion. Do we really need an option for that? Wouldn't increasing the default limit to 1024 be perfectly fine for everybody? I did not have any strong reason for choosing exactly 64 as the default: I wanted a number which is high enough to avoid reasonable programs running out of file descriptors and low enough to make it impossible for malicious programs to eat too much kernel memory.

brainoutsource commented 4 years ago

I'm working on evaluation of code submissions that allows dependencies/package managers. A code submission with its dependencies and transitive dependencies is a lot of files and I'd like to be in control of the limit.

Although I get that it's probably not the case for the majority of isolate users.

gollux commented 4 years ago

Still, isn't 1024 about one order of magnitude more than you can ever need?

Generally, it is cheap to add minor options like that, but once you add too many of them, it makes the code too convoluted and hard to audit.

brainoutsource commented 4 years ago

Again, I'd like to be in control of this number instead of setting it to a value that's "fine for everybody", because nothing really is. Personally, I want it to be lower or higher than 1024 depending on the circumstances. But hey, that's me, I can just build from my own fork.

pnshiralkar commented 4 years ago

Please consider adding this feature or atleast inscreasing the limit to something between 256 and 1024 instead of 64. R Language gives error when run in isolate due to this limit, however it runs fine in @brainoutsource 's fork with limit set to 256.

Thanks!

hermanzdosilovic commented 4 years ago

My two cents. I would love to see this feature in isolate. The code doesn't look "convoluted" to me and some reasonable default value for this optional setting would be great. What "reasonable" means? I would leave that decision to the maintainers. As isolate is the key component of every online judge I would expect it to be feature rich (which it already is). This is seems to me as a free feature that would extend use-cases where isolate can be used.