GNU bug report logs - #72337
Add /etc/subuid and /etc/subgid support

Previous Next

Full log

Message #44 received at 72337 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Giacomo Leidi <goodoldpaul <at> autistici.org>
Cc: 72337 <at> debbugs.gnu.org
Subject: Re: bug#72337: Add /etc/subuid and /etc/subgid support
Date: Wed, 04 Sep 2024 23:00:58 +0200

Giacomo Leidi <goodoldpaul <at> autistici.org> skribis:

> * gnu/build/accounts.scm (list-set): New variable;
> (%sub-id-min): new variable;
> (%sub-id-max): new variable;
> (%sub-id-count): new variable;
> (sub-id?): new variable;
> (subid-range-fits?): new variable;
> (subid-range-fits-between?): new variable;
> (insert-subid-range): new variable;
> (reserve-subids): new variable;
> (range->entry): new variable;
> (entry->range): new variable;
> (allocate-subids): new variable;
> (subuid+subgid-databases): new variable.
>
> * gnu/system/accounts.scm (subid-range-end): New variable;
> (subid-range-has-start?): new variable;
> (subid-range-less): new variable.
>
> * test/accounts.scm: Test them.
>
> Change-Id: I8de1fd7cfe508b9c76408064d6f498471da0752d

Woow, neat!  It didn’t occur to me that we’d need a proper subid
allocation mechanism as well.

> +(define (list-set lst el k)
> +  (if (>= k (length lst))
> +      `(,@lst ,el)
> +      `(,@(list-head lst k)
> +        ,el
> +        ,@(list-tail lst k))))

‘length’, ‘list-ref’, and thus ‘list-set’ are linear in the size of the
list so it’s something we should avoid, unless we know that the lists
we’re dealing with are always going to be small.

> +;; According to Shadow's libmisc/find_new_sub_uids.c and
> +;; libmisc/find_new_sub_gids.c.
> +(define %sub-id-min 100000)
> +(define %sub-id-max 600100000)
> +(define %sub-id-count 65536)

[...]

> +(define (sub-id? id)
> +  (and (>= id %sub-id-min)
> +       (< id %sub-id-max)))

s/sub-/subordinate-/

> +(define (subid-range-fits? r interval-start interval-end)
> +  (and (<= interval-start
> +           (subid-range-start r))
> +       (<= (subid-range-end r)
> +           interval-end)))

Maybe: (within-subordinate-id-range? start end range) ?

Also, shouldn’t the first <= be >= ?

Please add docstrings for top-level procedures.

> +(define (subid-range-fits-between? r a b)
> +  (subid-range-fits? r
> +                     (+ (subid-range-start a) 1)
> +                     (- (subid-range-end b) 1)))

Maybe: (containing-subordinate-id-range? range a b) ?

> +(define (insert-subid-range range lst)

We definitely need a docstring, I’m not sure what this is supposed to
do.  :-)

> +    (unless (and (sub-id? range-start)
> +                 (sub-id? range-end))
> +      (raise
> +       (string-append "Subid range of " range-name
> +                      " from " (number->string range-start) " to "
> +                      (number->string range-end)
> +                      " spans over illegal subids.  Max allowed is "
> +                      (number->string %sub-id-max) ", min is "
> +                      (number->string %sub-id-min) "."))))

There are two issues: first we need ‘raise’ from (srfi srfi-34), not
from (guile), since the latter has nothing to do with exceptions.

Second, ‘raise’ takes a SRFI-35 “error condition” (essentially a
record), not a string.

But my suggestion here would be to define specific error conditions,
like:

  (define-condition-type &subordinate-id-error &error)
  (define-condition-type &subordinate-id-range-error &subordinate-id-error
    (id subordinate-id-range-error-id))

The latter is what we’d use here.

This procedure uses lists a lot, which should probably be avoided as I
wrote above.  Perhaps a vlist would do, or perhaps a vhash, or a vector.

The procedure is also very long; I wonder if it could be further split
and/or share code with the existing allocation-related code.

> +  (test-error "allocate-subids with interleaving, impossible interleaving"
> +              "error"
> +              ;; Make sure it's impossible to explicitly request impossible allocations

Instead of ‘test-error’, which is currently kinda broken IIRC, I’d
suggest a more explicit approach:

  (test-assert …
    (guard (c ((whatever-error? c) #t))
      …
      #f))

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.