All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH] package/minijail: new package
@ 2022-01-13 10:05 José Pekkarinen
  2022-01-13 19:51 ` Thomas Petazzoni
  0 siblings, 1 reply; 3+ messages in thread
From: José Pekkarinen @ 2022-01-13 10:05 UTC (permalink / raw)
  To: buildroot; +Cc: José Pekkarinen

This patch adds package minijail

Minijail depends in a toolchain different from
uclibc thanks to it's lack of support for prlimits.

Signed-off-by: José Pekkarinen <jose.pekkarinen@unikie.com>
---
[ v1 -> v2 ]
- Fixed hash file
- Fixed static assert patch
- Depend in toolchain distinct of uclibc
- Remove redundant host libpcap dependency
- Remove redundant parenthesis on MINIJAIL_BUILD_CMDS

 DEVELOPERS                                    |  1 +
 package/Config.in                             |  1 +
 ...te-static_assert-with-_Static_assert.patch | 35 +++++++++++++++++++
 package/minijail/Config.in                    | 12 +++++++
 package/minijail/minijail.hash                |  5 +++
 package/minijail/minijail.mk                  | 28 +++++++++++++++
 6 files changed, 82 insertions(+)
 create mode 100644 package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
 create mode 100644 package/minijail/Config.in
 create mode 100644 package/minijail/minijail.hash
 create mode 100644 package/minijail/minijail.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index 55f37dfe62..3631021077 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1515,6 +1515,7 @@ F:	support/testing/tests/package/test_zfs.py
 N:	José Pekkarinen <jose.pekkarinen@unikie.com>
 F:	package/alfred/
 F:	package/bmx7/
+F:	package/minijail/
 F:	package/python-aexpect/
 F:	package/softhsm2/
 F:	support/testing/tests/package/sample_python_aexpect.py
diff --git a/package/Config.in b/package/Config.in
index a1924f96df..3b11b003fb 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -2556,6 +2556,7 @@ menu "System tools"
 	source "package/mender/Config.in"
 	source "package/mender-grubenv/Config.in"
 	source "package/mfoc/Config.in"
+	source "package/minijail/Config.in"
 	source "package/moby-buildkit/Config.in"
 	source "package/monit/Config.in"
 	source "package/multipath-tools/Config.in"
diff --git a/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
new file mode 100644
index 0000000000..ff85995114
--- /dev/null
+++ b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
@@ -0,0 +1,35 @@
+From 8a6d5a1c48b85fb49f0d68ec31ecc51fd22e7201 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Jos=C3=A9=20Pekkarinen?= <jose.pekkarinen@unikie.com>
+Date: Wed, 12 Jan 2022 17:09:27 +0200
+Subject: [PATCH] Substitute static_assert with _Static_assert
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+Substitute static_assert with _Static_assert
+
+static_assert behaves differently for uclibc
+toolchains. Substituting it with the standard
+_Static_assert builds on all toolchains tested.
+
+Signed-off-by: José Pekkarinen <jose.pekkarinen@unikie.com>
+---
+ libminijail.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/libminijail.c b/libminijail.c
+index b935dfd..b154f1c 100644
+--- a/libminijail.c
++++ b/libminijail.c
+@@ -2620,7 +2620,7 @@ static int fd_is_open(int fd)
+ 	return fcntl(fd, F_GETFD) != -1 || errno != EBADF;
+ }
+ 
+-static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1,
++_Static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1,
+ 	      "If true, ensure_no_fd_conflict will always find an unused fd.");
+ 
+ /* If parent_fd will be used by a child fd, move it to an unused fd. */
+-- 
+2.25.1
+
diff --git a/package/minijail/Config.in b/package/minijail/Config.in
new file mode 100644
index 0000000000..24d307ed54
--- /dev/null
+++ b/package/minijail/Config.in
@@ -0,0 +1,12 @@
+config BR2_PACKAGE_MINIJAIL
+	bool "minijail"
+	depends on !BR2_STATIC_LIBS # dlopen()
+	depends on !BR2_TOOLCHAIN_USES_UCLIBC
+	select BR2_PACKAGE_LIBCAP
+	help
+	  Minijail is a sandboxing tool maintained by google.
+
+	  https://google.github.io/minijail/
+
+comment "minijail needs a glibc or musl toolchain with dynamic library support"
+	depends on BR2_STATIC_LIBS || BR2_TOOLCHAIN_USES_UCLIBC
diff --git a/package/minijail/minijail.hash b/package/minijail/minijail.hash
new file mode 100644
index 0000000000..d9f497a86c
--- /dev/null
+++ b/package/minijail/minijail.hash
@@ -0,0 +1,5 @@
+# Locally computed from https://github.com/google/minijail/releases/
+sha256  1ee5a5916491a32c121c7422b4d8c16481c0396a3acab34bf1c44589dcf810ae  linux-v17.tar.gz
+
+# Locally computed
+sha256  c6f439c5cf07263f71f01d29b79c79172ee529088e51ab434b22baad0988fe57  LICENSE
diff --git a/package/minijail/minijail.mk b/package/minijail/minijail.mk
new file mode 100644
index 0000000000..78898865fb
--- /dev/null
+++ b/package/minijail/minijail.mk
@@ -0,0 +1,28 @@
+################################################################################
+#
+# minijail
+#
+################################################################################
+
+MINIJAIL_VERSION = linux-v17
+MINIJAIL_SOURCE = $(MINIJAIL_VERSION).tar.gz
+MINIJAIL_SITE = "https://github.com/google/minijail/archive/refs/tags"
+MINIJAIL_LICENSE = BSD-Style
+MINIJAIL_LICENSE_FILES = LICENSE
+MINIJAIL_DEPENDENCIES=libcap
+
+define MINIJAIL_BUILD_CMDS
+	cd $(@D); \
+	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/$(d) CC="$(TARGET_CC)"
+endef
+
+define MINIJAIL_INSTALL_TARGET_CMDS
+	$(INSTALL) -m 0755 -D $(@D)/minijail0 \
+		$(TARGET_DIR)/usr/bin/minijail0
+	$(INSTALL) -m 0755 -D $(@D)/libminijailpreload.so \
+		$(TARGET_DIR)/lib/libminijailpreload.so
+	$(INSTALL) -m 0755 -D $(@D)/libminijail.so \
+		$(TARGET_DIR)/lib/libminijail.so
+endef
+
+$(eval $(generic-package))
-- 
2.25.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Buildroot] [PATCH] package/minijail: new package
  2022-01-13 10:05 [Buildroot] [PATCH] package/minijail: new package José Pekkarinen
@ 2022-01-13 19:51 ` Thomas Petazzoni
  2022-01-14  5:50   ` José Pekkarinen
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Petazzoni @ 2022-01-13 19:51 UTC (permalink / raw)
  To: José Pekkarinen; +Cc: buildroot

Hello José,

Thanks for this new iteration! Comments below.

On Thu, 13 Jan 2022 12:05:06 +0200
José Pekkarinen <jose.pekkarinen@unikie.com> wrote:

> This patch adds package minijail
> 
> Minijail depends in a toolchain different from
> uclibc thanks to it's lack of support for prlimits.
> 
> Signed-off-by: José Pekkarinen <jose.pekkarinen@unikie.com>
> ---
> [ v1 -> v2 ]
> - Fixed hash file
> - Fixed static assert patch
> - Depend in toolchain distinct of uclibc
> - Remove redundant host libpcap dependency
> - Remove redundant parenthesis on MINIJAIL_BUILD_CMDS

If this is v2, your patch should have been generated with "git
format-patch -v2", so that its title is [PATCH v2] and not just [PATCH].

> diff --git a/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
> new file mode 100644
> index 0000000000..ff85995114
> --- /dev/null
> +++ b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
> @@ -0,0 +1,35 @@
> +From 8a6d5a1c48b85fb49f0d68ec31ecc51fd22e7201 Mon Sep 17 00:00:00 2001
> +From: =?UTF-8?q?Jos=C3=A9=20Pekkarinen?= <jose.pekkarinen@unikie.com>
> +Date: Wed, 12 Jan 2022 17:09:27 +0200
> +Subject: [PATCH] Substitute static_assert with _Static_assert
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=UTF-8
> +Content-Transfer-Encoding: 8bit
> +
> +Substitute static_assert with _Static_assert
> +
> +static_assert behaves differently for uclibc
> +toolchains. Substituting it with the standard
> +_Static_assert builds on all toolchains tested.

So this is fixing a problem affecting uClibc toolchains, and below you
exclude uClibc toolchains ? Does it make sense to have this patch ?

> diff --git a/package/minijail/Config.in b/package/minijail/Config.in
> new file mode 100644
> index 0000000000..24d307ed54
> --- /dev/null
> +++ b/package/minijail/Config.in
> @@ -0,0 +1,12 @@
> +config BR2_PACKAGE_MINIJAIL
> +	bool "minijail"
> +	depends on !BR2_STATIC_LIBS # dlopen()
> +	depends on !BR2_TOOLCHAIN_USES_UCLIBC

Please add a comment on top of this that explains why uClibc toolchains
are excluded (prlimit not implemented).

> +	select BR2_PACKAGE_LIBCAP

You need to replicate:

	depends on BR2_USE_MMU

from the list of libcap dependencies.

> +	help
> +	  Minijail is a sandboxing tool maintained by google.
> +
> +	  https://google.github.io/minijail/
> +
> +comment "minijail needs a glibc or musl toolchain with dynamic library support"

and also have it here.

> +	depends on BR2_STATIC_LIBS || BR2_TOOLCHAIN_USES_UCLIBC
> diff --git a/package/minijail/minijail.hash b/package/minijail/minijail.hash
> new file mode 100644
> index 0000000000..d9f497a86c
> --- /dev/null
> +++ b/package/minijail/minijail.hash
> @@ -0,0 +1,5 @@
> +# Locally computed from https://github.com/google/minijail/releases/
> +sha256  1ee5a5916491a32c121c7422b4d8c16481c0396a3acab34bf1c44589dcf810ae  linux-v17.tar.gz
> +
> +# Locally computed
> +sha256  c6f439c5cf07263f71f01d29b79c79172ee529088e51ab434b22baad0988fe57  LICENSE
> diff --git a/package/minijail/minijail.mk b/package/minijail/minijail.mk
> new file mode 100644
> index 0000000000..78898865fb
> --- /dev/null
> +++ b/package/minijail/minijail.mk
> @@ -0,0 +1,28 @@
> +################################################################################
> +#
> +# minijail
> +#
> +################################################################################
> +
> +MINIJAIL_VERSION = linux-v17
> +MINIJAIL_SOURCE = $(MINIJAIL_VERSION).tar.gz
> +MINIJAIL_SITE = "https://github.com/google/minijail/archive/refs/tags"

Please use the following lines instead:

MINIJAIL_VERSION = 17
MINIJAIL_SITE = $(call github,google,minijail,linux-v$(MINIJAIL_VERSION))

You will have to adjust the .hash file accordingly.

> +MINIJAIL_LICENSE = BSD-Style

This is not a license. See https://spdx.org/licenses/ for the list of
license identifiers that are valid.

> +MINIJAIL_LICENSE_FILES = LICENSE
> +MINIJAIL_DEPENDENCIES=libcap

Spaces around "=".

> +define MINIJAIL_BUILD_CMDS
> +	cd $(@D); \
> +	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/$(d) CC="$(TARGET_CC)"

No need to cd into $(@D), since you run make with -C $(@D). Please drop
the $(d) which is an empty variable. Also please use
$(TARGET_CONFIGURE_OPTS). So something like this:

	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D)

I'm pretty sure I already made this comment on a previous version, or
perhaps on another package contributed by you.

Could you take into account those comments and send a v3 ? It should be
good to merge then. Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Buildroot] [PATCH] package/minijail: new package
  2022-01-13 19:51 ` Thomas Petazzoni
@ 2022-01-14  5:50   ` José Pekkarinen
  0 siblings, 0 replies; 3+ messages in thread
From: José Pekkarinen @ 2022-01-14  5:50 UTC (permalink / raw)
  To: Thomas Petazzoni; +Cc: buildroot

On Thu, Jan 13, 2022 at 9:51 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello José,
>
> Thanks for this new iteration! Comments below.
>
> On Thu, 13 Jan 2022 12:05:06 +0200
> José Pekkarinen <jose.pekkarinen@unikie.com> wrote:
>
> > This patch adds package minijail
> >
> > Minijail depends in a toolchain different from
> > uclibc thanks to it's lack of support for prlimits.
> >
> > Signed-off-by: José Pekkarinen <jose.pekkarinen@unikie.com>
> > ---
> > [ v1 -> v2 ]
> > - Fixed hash file
> > - Fixed static assert patch
> > - Depend in toolchain distinct of uclibc
> > - Remove redundant host libpcap dependency
> > - Remove redundant parenthesis on MINIJAIL_BUILD_CMDS
>
> If this is v2, your patch should have been generated with "git
> format-patch -v2", so that its title is [PATCH v2] and not just [PATCH].

    Ack.

> > diff --git a/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
> > new file mode 100644
> > index 0000000000..ff85995114
> > --- /dev/null
> > +++ b/package/minijail/0001-Substitute-static_assert-with-_Static_assert.patch
> > @@ -0,0 +1,35 @@
> > +From 8a6d5a1c48b85fb49f0d68ec31ecc51fd22e7201 Mon Sep 17 00:00:00 2001
> > +From: =?UTF-8?q?Jos=C3=A9=20Pekkarinen?= <jose.pekkarinen@unikie.com>
> > +Date: Wed, 12 Jan 2022 17:09:27 +0200
> > +Subject: [PATCH] Substitute static_assert with _Static_assert
> > +MIME-Version: 1.0
> > +Content-Type: text/plain; charset=UTF-8
> > +Content-Transfer-Encoding: 8bit
> > +
> > +Substitute static_assert with _Static_assert
> > +
> > +static_assert behaves differently for uclibc
> > +toolchains. Substituting it with the standard
> > +_Static_assert builds on all toolchains tested.
>
> So this is fixing a problem affecting uClibc toolchains, and below you
> exclude uClibc toolchains ? Does it make sense to have this patch ?

    Ack.

> > diff --git a/package/minijail/Config.in b/package/minijail/Config.in
> > new file mode 100644
> > index 0000000000..24d307ed54
> > --- /dev/null
> > +++ b/package/minijail/Config.in
> > @@ -0,0 +1,12 @@
> > +config BR2_PACKAGE_MINIJAIL
> > +     bool "minijail"
> > +     depends on !BR2_STATIC_LIBS # dlopen()
> > +     depends on !BR2_TOOLCHAIN_USES_UCLIBC
>
> Please add a comment on top of this that explains why uClibc toolchains
> are excluded (prlimit not implemented).

    Ack.

> > +     select BR2_PACKAGE_LIBCAP
>
> You need to replicate:
>
>         depends on BR2_USE_MMU
>
> from the list of libcap dependencies.
>
> > +     help
> > +       Minijail is a sandboxing tool maintained by google.
> > +
> > +       https://google.github.io/minijail/
> > +
> > +comment "minijail needs a glibc or musl toolchain with dynamic library support"
>
> and also have it here.

    Ack.

> > +     depends on BR2_STATIC_LIBS || BR2_TOOLCHAIN_USES_UCLIBC
> > diff --git a/package/minijail/minijail.hash b/package/minijail/minijail.hash
> > new file mode 100644
> > index 0000000000..d9f497a86c
> > --- /dev/null
> > +++ b/package/minijail/minijail.hash
> > @@ -0,0 +1,5 @@
> > +# Locally computed from https://github.com/google/minijail/releases/
> > +sha256  1ee5a5916491a32c121c7422b4d8c16481c0396a3acab34bf1c44589dcf810ae  linux-v17.tar.gz
> > +
> > +# Locally computed
> > +sha256  c6f439c5cf07263f71f01d29b79c79172ee529088e51ab434b22baad0988fe57  LICENSE
> > diff --git a/package/minijail/minijail.mk b/package/minijail/minijail.mk
> > new file mode 100644
> > index 0000000000..78898865fb
> > --- /dev/null
> > +++ b/package/minijail/minijail.mk
> > @@ -0,0 +1,28 @@
> > +################################################################################
> > +#
> > +# minijail
> > +#
> > +################################################################################
> > +
> > +MINIJAIL_VERSION = linux-v17
> > +MINIJAIL_SOURCE = $(MINIJAIL_VERSION).tar.gz
> > +MINIJAIL_SITE = "https://github.com/google/minijail/archive/refs/tags"
>
> Please use the following lines instead:
>
> MINIJAIL_VERSION = 17
> MINIJAIL_SITE = $(call github,google,minijail,linux-v$(MINIJAIL_VERSION))
>
> You will have to adjust the .hash file accordingly.
>
> > +MINIJAIL_LICENSE = BSD-Style
>
> This is not a license. See https://spdx.org/licenses/ for the list of
> license identifiers that are valid.

    It's license doesn't reference any hint to the
existing in spdx, identifying an equivalent is lawyers
work.

> > +MINIJAIL_LICENSE_FILES = LICENSE
> > +MINIJAIL_DEPENDENCIES=libcap
>
> Spaces around "=".

    Ack.

> > +define MINIJAIL_BUILD_CMDS
> > +     cd $(@D); \
> > +     $(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/$(d) CC="$(TARGET_CC)"
>
> No need to cd into $(@D), since you run make with -C $(@D). Please drop
> the $(d) which is an empty variable. Also please use
> $(TARGET_CONFIGURE_OPTS). So something like this:
>
>         $(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) -C $(@D)
>
> I'm pretty sure I already made this comment on a previous version, or
> perhaps on another package contributed by you.

     Yes, sorry, I failed to find the email related.

> Could you take into account those comments and send a v3 ? It should be
> good to merge then. Thanks a lot!

    Happy to do so.

    Thanks!

    José.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-14  5:51 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13 10:05 [Buildroot] [PATCH] package/minijail: new package José Pekkarinen
2022-01-13 19:51 ` Thomas Petazzoni
2022-01-14  5:50   ` José Pekkarinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.