[Buildroot] [2018.02.x 1/2] makedevs: allow leading whitespace for capabilities

[Buildroot] [2018.02.x 1/2] makedevs: allow leading whitespace for capabilities

[Ricardo Martincoski]

Currently makedevs silently ignores extended attributes with leading
whitespace, for example those added to a <PACKAGE>_PERMISSIONS following
the recommended style from check-package.

Makedevs already ignores leading whitespace for normal entries (file
permission changes and device files creation). Do the same for extended
attributes.

Fixes: #11191.

Reported-by: Jean-pierre Cartal <jpcartal@free.fr>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/makedevs/makedevs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
index 1ba5936342..c57b964f5c 100644
--- a/package/makedevs/makedevs.c
+++ b/package/makedevs/makedevs.c
@@ -510,7 +510,7 @@ int main(int argc, char **argv)
 
 		linenum++;
 
-		if (1 == sscanf(line, "|xattr %254s", xattr)) {
+		if (1 == sscanf(line, " |xattr %254s", xattr)) {
 #ifdef EXTENDED_ATTRIBUTES
 			if (!full_name)
 				bb_error_msg_and_die("line %d should be after a file\n", linenum);
-- 
2.17.1

[Buildroot] [2018.02.x 2/2] support/testing: add test for file capabilities

[Ricardo Martincoski]

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
complete run on GitLab CI on top of 2018.02.4 with the series applied:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27226377

only the test on top of 2018.02.4:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985431

only the test on top of 2018.05.1:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985455

only the test on top of 2018.08-rc1:
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985484
---
 .gitlab-ci.yml                                |  1 +
 support/testing/tests/core/device_table2.txt  |  7 +++
 .../tests/core/squashfs-xattr-kernel.config   |  1 +
 .../tests/core/test_file_capabilities.py      | 47 +++++++++++++++++++
 4 files changed, 56 insertions(+)
 create mode 100644 support/testing/tests/core/device_table2.txt
 create mode 100644 support/testing/tests/core/squashfs-xattr-kernel.config
 create mode 100644 support/testing/tests/core/test_file_capabilities.py

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 59ce6ae59e..271b938842 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -237,6 +237,7 @@ zynq_zybo_defconfig: *defconfig
 tests.boot.test_atf.TestATFAllwinner: *runtime_test
 tests.boot.test_atf.TestATFMarvell: *runtime_test
 tests.boot.test_atf.TestATFVexpress: *runtime_test
+tests.core.test_file_capabilities.TestFileCapabilities: *runtime_test
 tests.core.test_post_scripts.TestPostScripts: *runtime_test
 tests.core.test_rootfs_overlay.TestRootfsOverlay: *runtime_test
 tests.core.test_timezone.TestGlibcAllTimezone: *runtime_test
diff --git a/support/testing/tests/core/device_table2.txt b/support/testing/tests/core/device_table2.txt
new file mode 100644
index 0000000000..f8323217f3
--- /dev/null
+++ b/support/testing/tests/core/device_table2.txt
@@ -0,0 +1,7 @@
+# <name>				<type>	<mode>	<uid>	<gid>	<major>	<minor>	<start>	<inc>	<count>
+/usr/sbin/getcap                        f       755     0       0	-	-	-	-	-
+|xattr cap_sys_nice+eip
+# leading spaces are ignored for xattr
+  |xattr cap_kill+eip
+# leading tabs are ignored for xattr
+	|xattr cap_sys_time+eip
diff --git a/support/testing/tests/core/squashfs-xattr-kernel.config b/support/testing/tests/core/squashfs-xattr-kernel.config
new file mode 100644
index 0000000000..81b21b94c5
--- /dev/null
+++ b/support/testing/tests/core/squashfs-xattr-kernel.config
@@ -0,0 +1 @@
+CONFIG_SQUASHFS_XATTR=y
diff --git a/support/testing/tests/core/test_file_capabilities.py b/support/testing/tests/core/test_file_capabilities.py
new file mode 100644
index 0000000000..945b48a4c3
--- /dev/null
+++ b/support/testing/tests/core/test_file_capabilities.py
@@ -0,0 +1,47 @@
+import os
+import subprocess
+
+import infra.basetest
+
+
+class TestFileCapabilities(infra.basetest.BRTest):
+    config = \
+        """
+        BR2_arm=y
+        BR2_TOOLCHAIN_EXTERNAL=y
+        BR2_ROOTFS_DEVICE_TABLE="system/device_table.txt {}"
+        BR2_ROOTFS_DEVICE_TABLE_SUPPORTS_EXTENDED_ATTRIBUTES=y
+        BR2_TARGET_GENERIC_GETTY_PORT="ttyAMA0"
+        BR2_LINUX_KERNEL=y
+        BR2_LINUX_KERNEL_CUSTOM_VERSION=y
+        BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="4.11.3"
+        BR2_LINUX_KERNEL_DEFCONFIG="vexpress"
+        BR2_LINUX_KERNEL_CONFIG_FRAGMENT_FILES="{}"
+        BR2_LINUX_KERNEL_DTS_SUPPORT=y
+        BR2_LINUX_KERNEL_INTREE_DTS_NAME="vexpress-v2p-ca9"
+        BR2_PACKAGE_LIBCAP=y
+        BR2_PACKAGE_LIBCAP_TOOLS=y
+        BR2_TARGET_ROOTFS_SQUASHFS=y
+        # BR2_TARGET_ROOTFS_TAR is not set
+        """.format(infra.filepath("tests/core/device_table2.txt"),
+                   infra.filepath("tests/core/squashfs-xattr-kernel.config"))
+
+    def test_run(self):
+        img = os.path.join(self.builddir, "images", "rootfs.squashfs")
+        subprocess.call(["truncate", "-s", "%1M", img])
+
+        self.emulator.boot(arch="armv7",
+                           kernel=os.path.join(self.builddir, "images", "zImage"),
+                           kernel_cmdline=["root=/dev/mmcblk0",
+                                           "rootfstype=squashfs"],
+                           options=["-drive", "file={},if=sd,format=raw".format(img),
+                                    "-M", "vexpress-a9",
+                                    "-dtb", os.path.join(self.builddir, "images", "vexpress-v2p-ca9.dtb")])
+        self.emulator.login()
+
+        cmd = "getcap -v /usr/sbin/getcap"
+        output, _ = self.emulator.run(cmd)
+        self.assertIn("cap_kill", output[0])
+        self.assertIn("cap_sys_nice", output[0])
+        self.assertIn("cap_sys_time", output[0])
+        self.assertIn("+eip", output[0])
-- 
2.17.1

[Buildroot] [2018.02.x 2/2] support/testing: add test for file capabilities

[Ricardo Martincoski]

Hello,

Please notice this test case could be applied to master or next, but it will
fail until we fix #11216.

On Mon, Aug 06, 2018 at 12:17 AM, Ricardo Martincoski wrote:

[snip]
> complete run on GitLab CI on top of 2018.02.4 with the series applied:
> https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27226377

https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985522 
passed, #11191 fixed

> only the test on top of 2018.02.4:
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985431

failed, detects #11191

> only the test on top of 2018.05.1:
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985455

failed, detects #11216

> only the test on top of 2018.08-rc1:
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985484

failed, detects #11216


Regards,
Ricardo

[Buildroot] [2018.02.x 2/2] support/testing: add test for file capabilities

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 > ---
 > complete run on GitLab CI on top of 2018.02.4 with the series applied:
 > https://gitlab.com/RicardoMartincoski/buildroot/pipelines/27226377

 > only the test on top of 2018.02.4:
 > https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985431

 > only the test on top of 2018.05.1:
 > https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985455

 > only the test on top of 2018.08-rc1:
 > https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/86985484

I know it fails on master, but then we are atleast aware of it, so:

Committed to master, and then backported to 2018.02.x and 2018.05.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [2018.02.x 1/2] makedevs: allow leading whitespace for capabilities

[Thomas Petazzoni]

Hello Ricardo,

On Mon,  6 Aug 2018 00:17:14 -0300, Ricardo Martincoski wrote:
> Currently makedevs silently ignores extended attributes with leading
> whitespace, for example those added to a <PACKAGE>_PERMISSIONS following
> the recommended style from check-package.
> 
> Makedevs already ignores leading whitespace for normal entries (file
> permission changes and device files creation). Do the same for extended
> attributes.
> 
> Fixes: #11191.
> 
> Reported-by: Jean-pierre Cartal <jpcartal@free.fr>
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

It is not clear to me why you have the "2018.02.x" prefix in your patch
title. I would assume this patch is needed in master, not just in
2018.02.x. We generally include 2018.02.x in the patch title when
the patch should only be applied to 2018.02.x. For patches going in
master, and needing a backport, we normally rely on Peter being smart
and realizing that the patch is a fix that makes sense for 2018.02.x.

Could you clarify what was your intention with this 2018.02.x prefix ?

Perhaps we need to have some tags ?

Backport-to: 2018.02.x

when the patch is for master, but needs to be backported. And perhaps:

Apply-to: 2018.02.x

when the patch is to be applied only on 2018.02.x.

Or simply rely on informal notes below the "---" sign ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [2018.02.x 1/2] makedevs: allow leading whitespace for capabilities

[Ricardo Martincoski]

Hello,

On Thu, Aug 09, 2018 at 06:42 PM, Thomas Petazzoni wrote:

> On Mon,  6 Aug 2018 00:17:14 -0300, Ricardo Martincoski wrote:
>> Currently makedevs silently ignores extended attributes with leading
>> whitespace, for example those added to a <PACKAGE>_PERMISSIONS following
>> the recommended style from check-package.
>> 
>> Makedevs already ignores leading whitespace for normal entries (file
>> permission changes and device files creation). Do the same for extended
>> attributes.
>> 
>> Fixes: #11191.
>> 
>> Reported-by: Jean-pierre Cartal <jpcartal@free.fr>
>> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
>> Cc: Arnout Vandecappelle <arnout@mind.be>
>> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> It is not clear to me why you have the "2018.02.x" prefix in your patch
> title. I would assume this patch is needed in master, not just in
> 2018.02.x. We generally include 2018.02.x in the patch title when
> the patch should only be applied to 2018.02.x. For patches going in
> master, and needing a backport, we normally rely on Peter being smart
> and realizing that the patch is a fix that makes sense for 2018.02.x.
> 
> Could you clarify what was your intention with this 2018.02.x prefix ?

I developed and runtime tested the patch for 2018.02.x.
But now I tested the patch (by temporarily adding a printf to makedevs) in the
master branch too. It works well.

This patch can/should be applied to master.

In the branch 2018.02.x this patch unbreaks xattr with leading whitespace.
In the master branch (and also 2018.05.x), which use a per-fs target dir, there
are other issues with xattr, this patch is only part of the solution to unbreak
xattrs, see #11216.

> 
> Perhaps we need to have some tags ?
> 
> Backport-to: 2018.02.x
> 
> when the patch is for master, but needs to be backported. And perhaps:
> 
> Apply-to: 2018.02.x
> 
> when the patch is to be applied only on 2018.02.x.
> 
> Or simply rely on informal notes below the "---" sign ?

Sorry, I should have added such informal notes.

Regards,
Ricardo

[Buildroot] [2018.02.x 1/2] makedevs: allow leading whitespace for capabilities

[Peter Korsgaard]

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Hello,
 > On Thu, Aug 09, 2018 at 06:42 PM, Thomas Petazzoni wrote:

 >> On Mon,  6 Aug 2018 00:17:14 -0300, Ricardo Martincoski wrote:
 >>> Currently makedevs silently ignores extended attributes with leading
 >>> whitespace, for example those added to a <PACKAGE>_PERMISSIONS following
 >>> the recommended style from check-package.
 >>> 
 >>> Makedevs already ignores leading whitespace for normal entries (file
 >>> permission changes and device files creation). Do the same for extended
 >>> attributes.
 >>> 
 >>> Fixes: #11191.
 >>> 
 >>> Reported-by: Jean-pierre Cartal <jpcartal@free.fr>
 >>> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
 >>> Cc: Arnout Vandecappelle <arnout@mind.be>
 >>> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
 >> 
 >> It is not clear to me why you have the "2018.02.x" prefix in your patch
 >> title. I would assume this patch is needed in master, not just in
 >> 2018.02.x. We generally include 2018.02.x in the patch title when
 >> the patch should only be applied to 2018.02.x. For patches going in
 >> master, and needing a backport, we normally rely on Peter being smart
 >> and realizing that the patch is a fix that makes sense for 2018.02.x.
 >> 
 >> Could you clarify what was your intention with this 2018.02.x prefix ?

 > I developed and runtime tested the patch for 2018.02.x.
 > But now I tested the patch (by temporarily adding a printf to makedevs) in the
 > master branch too. It works well.

 > This patch can/should be applied to master.

 > In the branch 2018.02.x this patch unbreaks xattr with leading whitespace.
 > In the master branch (and also 2018.05.x), which use a per-fs target dir, there
 > are other issues with xattr, this patch is only part of the solution to unbreak
 > xattrs, see #11216.

Ok, thanks.

Committed to master, and then backported to 2018.02.x and 2018.05.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [2018.02.x 1/2] makedevs: allow leading whitespace for capabilities

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@bootlin.com> writes:

Hi,

 > It is not clear to me why you have the "2018.02.x" prefix in your patch
 > title. I would assume this patch is needed in master, not just in
 > 2018.02.x. We generally include 2018.02.x in the patch title when
 > the patch should only be applied to 2018.02.x. For patches going in
 > master, and needing a backport, we normally rely on Peter being smart
 > and realizing that the patch is a fix that makes sense for 2018.02.x.

 > Could you clarify what was your intention with this 2018.02.x prefix ?

 > Perhaps we need to have some tags ?

 > Backport-to: 2018.02.x

 > when the patch is for master, but needs to be backported. And perhaps:

 > Apply-to: 2018.02.x

 > when the patch is to be applied only on 2018.02.x.

 > Or simply rely on informal notes below the "---" sign ?

Just stating it below the --- sign is IMHO good enough, and in the worst
case send a mail if I miss something.

-- 
Bye, Peter Korsgaard