Ver.6 (Re: [PATCH 7/8] busybox -- SELinux option support for coreutils: ver3)

KaiGai Kohei kaigai at kaigai.gr.jp
Sat Mar 10 05:43:01 UTC 2007


Hello, Thanks for your reviewing.

>> * '\n' was replaced by '0xff'
>> * Checks for exclusive options were added. chcon applet will will abort
>>  if --reference and '-u','-r','-t','-l' are appeared concurrently,
> 
> Please move the change_dir_context() function around to remove the
> forward decl.

I replaced change_dir_context() function by recursive_action(), not only
its declaration.

> +static context_t compute_context_from_mask(security_context_t context,
> +       context_free (new_context);
> 
> surplus space
> Smaller if you goto ret where
> ret:
> 	return NULL;
> }

As you say, we can share the above function between runcon and chcon,
so I removed the compute_context_from_mask() and added a new function
in libbb/selinux_common.c.
See, busybox-coreutils-libbb-09.v6.patch

> +       context_string = context_str(context);
> +       if (file_context == NULL || strcmp(context_string, file_context) != 0) {
> 
> Can context_str() return NULL?

Yes, it can return NULL, if malloc() used internally didn't succeed.
So, I added the following check:

+        context_string = context_str(context);
+        if (!context_string) {
+                bb_error_msg("couldn't obtain security context in text expression");
+                goto skip;
+        }

> +               int fail = 0;
> 
> superfluous set. Remove initialization to 0

Fixed,

> +
> +               if (opts & OPT_NODEREFERENCE) {
> +                       fail = lsetfilecon (fname, context_string);
> +               } else {
> +                       fail = setfilecon (fname, context_string);
> +               }
> +               if ((opts & OPT_VERBOSE)
> +                   || ((opts & OPT_CHANHES) && !fail)) {
> +                       printf(!fail
> 
> bb_perror_msg() instead of printf?

bb_perror_msg() prints a message into stderr, although upstreamed chcon
prints a message into stdout. It will make an incompatibility.
So, I didn't use bb_error_msg() here.

> +                              ? "context of %s changed to %s\n"
> +                              : "failed to change context of %s to
> %s\n",
> +                              fname, context_string);
> +               }
> 
> change_dir_context() sounds a bit overdone. Don't we have an opendir
> wrapper with diagnostics? Perhaps this is a candidate for
> recursive_action() 

It was replaced by recursive_action().

> Manually inlining compute_context_from_mask() smaller?

It was moved into libbb/selinux_common.o to share with 'runcon'.

> Smaller to resuse the global option_mask32 rather than using opts in
> chcon_main() ?

I replaced any local 'opts' by option_mask32.

> +               ":?:f--v:v--f"      /* 'verbose' and 'quiet' are exclusive */
> Sounds like there is the word mutual missing.

'-f' is a option for the silent purpose.
It is a definition of upstreamed chcon.

Thanks,
-- 
KaiGai Kohei <kaigai at kaigai.gr.jp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-libbb-09.v6.patch
Type: text/x-diff
Size: 2083 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070310/812157ed/attachment-0006.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-runcon-08.v6.patch
Type: text/x-diff
Size: 4150 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070310/812157ed/attachment-0007.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-coreutils-chcon-07.v6.patch
Type: text/x-diff
Size: 5322 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20070310/812157ed/attachment-0008.bin 


More information about the busybox mailing list