Re: [Xen-devel] [PATCH v2 6/6] RFC: tools/dm_restrict: Enable QEMU sandboxing

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/24/2018 12:21 PM, Ian Jackson wrote:
> Apropos of our conversation on IRC, I looked at the checker script in
> detail.
> 
>> #!/bin/bash
>>
>> domain="$1"
> 
> Just noticed this, but: OMG no `set -e'.
> You probably want `set -o pipefail' too.

`set -e` never made any sense to me -- that's not the way I code in any
other language; why would scripting be any different?  What's the
advantage of doing that in the current script?

>> dmpid=$(xenstore-read /local/domain/$domid/image/device-model-pid 
>> 2>/dev/null)
>> if [[ -z "$dmpid" ]] ; then
>>     echo "xenstore-read failed"
>>     exit 1
>> fi
> 
> Why do you throw away the stderr from xenstore-read ?

That's left over from a previous version of the script, where I didn't
check to see whether $1 was numeric, but rather tried to interpret it as
numeric and if it failed, then ran `xl domid` on it.  I can take that out.

> 
>> failed="false"
> 
> Quotes are not needed here and seem un-idiomatic to me when the RHS is
> a simple atom.

Sure.

>> # TEST: Process / group id
> ...
>> # FIXME: deal with other UID configurations?
> 
> Since the test will fail if you don't do this, I think this is very
> sub-critical and you could drop the fixme.

This was really a question for the reviewer; probably it should have
been RFC rather than FIXME.

The question is: should the script handle the case where
`xen-qemuuser-shared` is defined instead of `xen-qemuuser-range-base`?

>> # Example input:
>> # Uid:       1193    1193    1193    1193
>> input=$(grep Uid /proc/$dmpid/status)
> 
> I have commented on this grep, and the subsequent regexpery, already.
> 
> But also: you check the uid and the gid, but by duplicating the code.
> Surely this could be a shell function.
> 
>> echo -n "Process UID: "
> 
> If this had been me, I would have written
>    begin_test "Process UID"
> and then
> 
>>     result="PASSED"
> 
>       test_passed
> 
>>     for i in {1..4}; do
>>      if [[ "${BASH_REMATCH[$i]}" != "$tgt_uid" ]] ; then
>>          result="FAILED"
>>          failed="true"
> 
> In particular, you open-code setting `failed' everywhere.  If you miss
> one then that could hide a test failure.  So
>             test_failed
> But you want to print a reason so
>             test_failed "got $uidorgid $actual_id wanted $tgt_id"
> 
> As a bonus, doing this now means you can fix the test output format to
> be more parseable by changing the code in one place.
> 
>>     if [[ "$root" != "$tgt_chroot" ]] ; then
>>      echo "FAILED"
> 
> You could introduce
>             test_condition 'root directory' "$root" != "$tgt_chroot"
> which calls test_passed or test_failed as appropriate.
> 
> If you have it return the same exit status as the test, you can use it
> for the uids where you would say
>         test_condition "one $uidorgid" $actual_id = $tgt_id || break
> and the rest of the time you would have to write
>             test_condition 'root directory' "$root" != "$tgt_chroot" ||:

I'll take a look at doing this if we decide to stick with bash.

>> function check_rlimit() {
>>     limit_name=$1
>>     limit_string=$2
>>     tgt=$3
>>
>>     echo -n "rlimit $limit_name: "
>>     input=$(grep "^$limit_string" /proc/$dmpid/limits)
> ...
>>     if [[ "$input" =~ 
>> ^$limit_string[[:space:]]*([^[:space:]]+)[[:space:]]*([^[:space:]]+)[[:space:]]*[^[:space:]]+
>>  ]] ; then
> 
> Because of the unfortunate format of /proc/PID/limits, you do can't
> just do the
>     fields=($input)
> trick but
>     fields=(${input#*  })

What will this do?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel