[pkg-apparmor] Bug#917874: Bug#917874: /etc/init.d/apparmor: 60: shift: can't shift that many

[Christian Boltz]

Hello,

Am Montag, 31. Dezember 2018, 13:04:59 CET schrieb Jakub Wilk:
> * Christian Boltz <debian-bugs at cboltz.de>, 2018-12-31, 12:22:
> >>The init script is broken. The start action fails with:
> >>   /etc/init.d/apparmor: 60: shift: can't shift that many
> >
> >This looks like a regression from upstream commit
> >0d5ab43d592245d011b2614e6e20fc7cb851c53c which got backported into
> >the Debian package.
> >
> >
> >The fix is most likely (untested, because I don't see this error on
> >openSUSE):
> I guess it's a bash vs dash thing:
> 
>    $ bash -c 'shift; echo $?'
>    1
> 
>    $ dash -c 'shift; echo $?'
>    dash: 1: shift: can't shift that many
> 
> I have /bin/sh pointing to dash (which is the Debian default).

Indeed, that could explain it - openSUSE defaults to bash for /bin/sh,
so it fails silently (and $? gets ignored).

> >-       if ! is_apparmor_present ; then
> >+       if ! is_apparmor_present apparmor ; then
> 
> Now I get:
> 
>    Starting AppArmor - failed, To enable AppArmor, ensure your kernel
> is configured with CONFIG_SECURITY_APPARMOR=y then add
> 'security=apparmor apparmor=1' to the kernel command line
> 
> It looks like is_apparmor_present() always fails?

Right :-(

(As a sidenote - I wonder why I don't see this failure on openSUSE + 
latest rc.apparmor.functions.  Intrigeri, do you have an idea what 
Debian is doing differently? If in doubt, a   sh -x   trace might be
interesting to see. A wild guess is that the
    if [ -f "${SECURITYFS}/${MODULE}/profiles" ]; then
test in is_apparmor_loaded() might fail for whatever reason so the 
"return" shortcut that avoids calling is_apparmor_present() doesn't get
taken.)


Back to the bug - the failure is caused by this test:

    [ $? -ne 0 -a -d /sys/module/apparmor ]

Note that the check uses   -ne   so it expects that $? != 0.
However, the while loop always ends with $? = 0, so the test always 
fails.


I'll have to do a little history lesson to explain what happened ;-)

The old version of the function was:

# Test if the apparmor "module" is present.
is_apparmor_present() {
    local modules=$1
    shift

    while [ $# -gt 0 ] ; do
        modules="$modules|$1"
        shift
    done

    # check for subdomainfs version of module
    grep -qE "^($modules)[[:space:]]" /proc/modules

    [ $? -ne 0 -a -d /sys/module/apparmor ]

    return $?
}

The function was called in two ways:
    is_apparmor_present apparmor
    is_apparmor_present apparmor subdomain
so basically /proc/modules was grepped for "apparmor" and "subdomain".
Since AppArmor gets built into the kernel (not as a module) since many 
years, that grep never found anything in /proc/modules and returned 
with $? = 1. (We could also have called "false" instead of grep - same 
result with less CPU cycles ;-)

The cleanup ("because AppArmor wasn't a kernel module since many years") 
removed that seemingly superfluous and pointless grep.

However, removing the grep means that $? no longer gets set to 1 (the 
while loop ends up with $? = 0), and that means that the function always 
fails.


That all said - can you please test with this patch?

--- a/parser/rc.apparmor.functions
+++ b/parser/rc.apparmor.functions
@@ -61,15 +62,9 @@ STATUS=0
 
 # Test if the apparmor "module" is present.
 is_apparmor_present() {
-       local modules=$1
-       shift
+       # TODO: change callers - handing over a parameter is no longer needed
 
-       while [ $# -gt 0 ] ; do
-               modules="$modules|$1"
-               shift
-       done
-
-       [ $? -ne 0 -a -d /sys/module/apparmor ]
+       [ -d /sys/module/apparmor ]
 
        return $?
 }


The function should now look like this:

is_apparmor_present() {
    # TODO: change callers - handing over a parameter is no longer needed

    [ -d /sys/module/apparmor ]

    return $?
}

> >the only code that still actually does something there is
> >
> >    [ -d /sys/module/apparmor ]
> 
> With this one-line implementation of is_apparmor_present(), the script
> seems to work, but I get warnings about missing functions:
> 
>    /etc/init.d/apparmor: 209: /etc/init.d/apparmor:
> aa_log_action_start: not found /etc/init.d/apparmor: 221:
> /etc/init.d/apparmor: aa_log_action_end: not found

That could be a Debian-specific issue - I'll let intrigeri handle this 
part ;-)  (a quick look indicates that these messages are "only" 
annoying, but shouldn't break anything)


Regards,

Christian Boltz
-- 
Honestly - I ignore every thread Basil or Linda take part in. I wonder
if we can somehow automate that. Perhaps a opensuse-factory-sane list
that is feed by opensuse-factory posts but automatically filters such
threads and posts? [Stephan Kulow in opensuse-factory]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://alioth-lists.debian.net/pipermail/pkg-apparmor-team/attachments/20181231/f468056b/attachment-0001.sig>