[libcamera-devel] [PATCH 2/3] test: log/process: check CAP_SYS_ADMIN in test init

[Laurent Pinchart]

Hi again,

On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote:
> Hi You-Sheng,
> 
> Thank you for the patch.
> 
> On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote:
> > While these tests may be executed as normal user at build time,
> > unshare() call will fail and so are tests log_process and process_test.
> > This change checks if one is granted with necessary capabilities so that
> > we don't fail the build unexpectedly.
> > 
> > Signed-off-by: You-Sheng Yang <vicamo.yang at canonical.com>
> > ---
> >  test/log/log_process.cpp      | 20 ++++++++++++++++++++
> >  test/log/meson.build          |  2 +-
> >  test/meson.build              |  2 ++
> >  test/process/meson.build      |  2 +-
> >  test/process/process_test.cpp | 23 +++++++++++++++++++++++
> >  5 files changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
> > index d46d5e3..876da22 100644
> > --- a/test/log/log_process.cpp
> > +++ b/test/log/log_process.cpp
> > @@ -9,6 +9,7 @@
> >  #include <iostream>
> >  #include <random>
> >  #include <string.h>
> > +#include <sys/capability.h>
> >  #include <sys/stat.h>
> >  #include <sys/types.h>
> >  #include <unistd.h>
> > @@ -55,6 +56,25 @@ class LogProcessTest : public Test
> >  protected:
> >  	int init()
> >  	{
> > +		int ret = TestPass;
> > +
> > +		cap_t caps = cap_get_proc();
> > +		if (caps == NULL) {
> > +			cerr << "failed to check process capabilities" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Check required permissions: CAP_SYS_ADMIN: unshare */
> > +		cap_flag_value_t fv;
> > +		if ((cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &fv) < 0) || (fv != CAP_SET)) {
> > +			cerr << "skip due to insufficient capability" << endl;
> > +			ret = TestSkip;
> > +		}
> 
> Would it make sense to add this as a helper function to the base Test
> class ?
> 
> > +
> > +		cap_free(caps);
> > +		if (ret != TestPass)
> > +			return ret;
> > +
> >  		random_device random;
> >  		num_ = random();
> >  		logPath_ = "/tmp/libcamera.worker.test." +
> > diff --git a/test/log/meson.build b/test/log/meson.build
> > index 8cd664e..000f980 100644
> > --- a/test/log/meson.build
> > +++ b/test/log/meson.build
> > @@ -7,7 +7,7 @@ log_test = [
> >  
> >  foreach t : log_test
> >      exe = executable(t[0], t[1],
> > -                     dependencies : libcamera_dep,
> > +                     dependencies : [libcamera_dep, libcap],
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_internal)
> >  
> > diff --git a/test/meson.build b/test/meson.build
> > index f41d6e7..b4db328 100644
> > --- a/test/meson.build
> > +++ b/test/meson.build
> > @@ -1,5 +1,7 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> > +libcap = dependency('libcap', required : true)
> 
> 'true' is the default value for 'required', you can omit it. However,
> I'd like to keep the dependency optional, as we try to also support
> resource-constrainted embedded systems (based on musl or uclibc for
> instance, and/or without udev).
> 
> I have an idea how to do that, I'll try to submit a patch shortly.

Actually, thinking about it some more, would it make sense to instead
condition the call to unshare() to CAP_SYS_ADMIN in the
Process:isolate() class ? Or turn it into a non-fatal error ?

Could you maybe elaborate a little bit on the failure this patch is
trying to solve ? I haven't seen any such failure, how can they be
reproduced ?

> > +
> >  subdir('libtest')
> >  
> >  subdir('camera')
> > diff --git a/test/process/meson.build b/test/process/meson.build
> > index c215fa7..828c17b 100644
> > --- a/test/process/meson.build
> > +++ b/test/process/meson.build
> > @@ -6,7 +6,7 @@ process_tests = [
> >  
> >  foreach t : process_tests
> >      exe = executable(t[0], t[1],
> > -                     dependencies : libcamera_dep,
> > +                     dependencies : [libcamera_dep, libcap],
> >                       link_with : test_libraries,
> >                       include_directories : test_includes_internal)
> >  
> > diff --git a/test/process/process_test.cpp b/test/process/process_test.cpp
> > index ce0cc7c..ffa2143 100644
> > --- a/test/process/process_test.cpp
> > +++ b/test/process/process_test.cpp
> > @@ -5,6 +5,8 @@
> >   * process_test.cpp - Process test
> >   */
> >  
> > +#include <sys/capability.h>
> > +
> >  #include <iostream>
> >  #include <unistd.h>
> >  #include <vector>
> > @@ -41,6 +43,27 @@ public:
> >  	}
> >  
> >  protected:
> > +	int init()
> > +	{
> > +		int ret = TestPass;
> > +
> > +		cap_t caps = cap_get_proc();
> > +		if (caps == NULL) {
> > +			cerr << "failed to check process capabilities" << endl;
> > +			return TestFail;
> > +		}
> > +
> > +		/* Check required permissions: CAP_SYS_ADMIN: unshare */
> > +		cap_flag_value_t fv;
> > +		if ((cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &fv) < 0) || (fv != CAP_SET)) {
> > +			cerr << "skip due to insufficient capability" << endl;
> > +			ret = TestSkip;
> > +		}
> > +
> > +		cap_free(caps);
> > +		return ret;
> > +	}
> > +
> >  	int run()
> >  	{
> >  		EventDispatcher *dispatcher = Thread::current()->eventDispatcher();

-- 
Regards,

Laurent Pinchart