Allow compilation & running in a different chroot - #3653
Conversation
58da773 to
790dc1b
Compare
meisterT
left a comment
meisterT
left a comment
There was a problem hiding this comment.
I think this is missing a change to the sudoers config as well
| while getopts "c:" opt; do | ||
| case $opt in | ||
| c) | ||
| CHROOTDIR="${CHROOTDIR//domjudge/$OPTARG}" |
There was a problem hiding this comment.
What do you think about handling $OPTARG relative to $(dirname "$CHROOTDIR") (or checking if it's an absolute path) rather than doing string substitution on domjudge?
One catch with ${CHROOTDIR//domjudge/$OPTARG} is that if a language has no custom chroot set it replaces domjudge with default and looks for /chroot/default instead of the original /chroot/domjudge. It would also be tricky if someone configured --with-judgehost-chrootdir with a path that doesn't include the literal word domjudge.
Perhaps something like:
if [ -z "$OPTARG" ] || [ "$OPTARG" = "default" ]; then
# keep default CHROOTDIR
:
elif [[ "$OPTARG" = /* ]]; then
CHROOTDIR="$OPTARG"
else
CHROOTDIR="$(dirname "$CHROOTDIR")/$OPTARG"
fi
What are your thoughts?
| set -e | ||
| } | ||
|
|
||
| CHROOTDIR_OPT="" |
There was a problem hiding this comment.
Looks like CHROOTDIR_OPT might be unused?
| @@ -119,9 +136,9 @@ case "$1" in | |||
| rmdir dev || true | |||
|
|
|||
| for i in $SUBDIRMOUNTS ; do | |||
There was a problem hiding this comment.
In stop, checking $CHROOTDIR/$i to decide whether to unmount or remove symlinks could be slightly fragile if stop is called without -c or with a different chroot than the one currently mounted.
Could we check the target in $PWD directly instead? That way stop wouldn't need to know which chroot was originally mounted to clean up properly.
| if ($chroot_run !== $this->chroot_current) { | ||
| logmsg(LOG_INFO, " 🔏 Submission should be done in different chroot '" . $chroot_run . "', leaving chroot '" . $this->chroot_current . "'"); | ||
| logmsg(LOG_INFO, " 🔓 Executing chroot script: '" . self::CHROOT_SCRIPT . " stop'"); | ||
| sleep(1); |
There was a problem hiding this comment.
Are all the sleep(1)s intentional or debugging? We optimized them out years ago and this regresses the state.
| return Verdict::INTERNAL_ERROR; | ||
| } | ||
|
|
||
| $chroot_run = $run_config['chroot'] ?? 'default'; |
There was a problem hiding this comment.
are we missing something like this here?
$this->chroot_current = $chroot_run;
| } | ||
|
|
||
| $chroot_compare = $run_config['chroot'] ?? 'default'; | ||
| if ($chroot_compare !== $this->chroot_current) { |
There was a problem hiding this comment.
At this point in execution, the working directory has already been changed to $realWorkdir if I am not mistaken so running chroot-startstop.sh here would try to mount inside the pass subdirectory rather than the judging workdir.
| } | ||
|
|
||
| $chroot_compile = $compile_config['chroot'] ?? 'default'; | ||
| if ($chroot_compile !== $this->chroot_current) { |
There was a problem hiding this comment.
We should extract all of this duplication into some helper logic.
| #[Serializer\Exclude] | ||
| private Collection $problems; | ||
|
|
||
| #[ORM\Column(length: 32, nullable: true, options: ['comment' => 'Custom chroot for executable'])] |
There was a problem hiding this comment.
For now, we should probably add Serializer\Exclude.
There was a problem hiding this comment.
Perhaps also #[Assert\Regex] constraint (e.g. ^[a-zA-Z0-9_-]+$) to ensure only clean directory names?
| private Collection $problems; | ||
|
|
||
| #[ORM\Column(length: 32, nullable: true, options: ['comment' => 'Custom chroot for executable'])] | ||
| private ?string $chroot_directory; |
| 'required' => false, | ||
| ]); | ||
| $builder->add('chrootDirectory', TextType::class, [ | ||
| 'required' => false, |
There was a problem hiding this comment.
Let's add a descriptive help message here.
2 participants
We discussed this a bit during NWERC, it would be nice to have a different chroot for specific problems or languages, this is the first step towards that goal.
It does uncover an interesting thing with either my setup or a race condition (with this or old code) somewhere. In case you shut down the judgedaemon with
ctrl+ceven if the judgedaemon is idle it complains that thechroot-start-stopcan´t remove all directories.