tags 580136 + patch thanks On Fri, May 07, 2010 at 05:04:51PM +0200, Arnaud Patard wrote: > Roger Leigh <rle...@codelibre.net> writes: > > > Could the Debian ARM list/porters possibly comment upon this > > bug? Please could you keep buildd-tools-devel and the bug > > in the CC on any reply. Thanks. > > > > It's not clear to me why this bug has suddenly appeared, and > > only on armel. There seem to be a few possibilities: > > > > 1) schroot is using personality(2) incorrectly, but this is > > only causing breakage on armel because only armel sets > > some of the high bits outside the range of PER_MASK > > 2) there's an armel-specific bug in the glibc personality > > wrapper and/or headers > > 3) there's an armel-specific bug in the kernel and/or its > > headers > > I would rather say it's a little bit more complicated than that. From > what I understand : > > by default, the personality is PER_LINUX_32BIT on eabi (and PER_LINUX on > oabi [1]), but : > > - on arm < v6, there's no xn/nx which means that READ_IMPLIES_EXEC will be > set. So personality(0xffffffff) gives 0x00c00000 > - on arm >= v6, there's xn/nx so READ_IMPLIES_EXEC will not be set and then > personality(0xffffffff) gives 0x00800000 > > It was giving 0x00800000 on all systems before because it was > broken. It has been fixed in the kernel with commit : > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=9da616fb9946c8d65387052e5a538b8f54ddb292
OK, thanks. It looks like we can't rely on personality(0xffffffff) ever giving a usable result. Given that some personality defines set the high bits and reuse the low bits, we can't mask out the high bits and get a sensible result... i.e. you can't roundtrip from personality(PER_OSR5) and get the same result back if you use PER_MASK, and the same applies to any kernel which sets PER_LINUX with any additional flags. It would be nice if some more thought had been given to this interface by the kernel folks, but it looks like we just can't reliably retrieve the personality from the kernel. Preferably every byte should resolve to a separate system type, but they aren't unique. I've attached a patch for schroot which removes the need for getting the personality from the kernel. We cache the set personality and return that instead. Not as nice as I would have liked, but it should fix up this issue. Please could someone test if this builds correctly on armel for me? If so, I'll include this fix in the next upload. This also needs testing on a 64bit system to ensure that personality=linux32 etc. still work correctly; if anyone could do that as well, it would be much appreciated. Many thanks, Roger -- .''`. Roger Leigh : :' : Debian GNU/Linux http://people.debian.org/~rleigh/ `. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/ `- GPG Public Key: 0x25BFB848 Please GPG sign your mail.
diff --git a/sbuild/sbuild-chroot-facet-personality.cc b/sbuild/sbuild-chroot-facet-personality.cc
index 906825d..b3c5c9b 100644
--- a/sbuild/sbuild-chroot-facet-personality.cc
+++ b/sbuild/sbuild-chroot-facet-personality.cc
@@ -28,13 +28,7 @@ using namespace sbuild;
chroot_facet_personality::chroot_facet_personality ():
chroot_facet(),
- persona(
-#ifdef __linux__
- personality("linux")
-#else
- personality("undefined")
-#endif
- )
+ persona()
{
}
@@ -99,8 +93,10 @@ void
chroot_facet_personality::get_keyfile (chroot const& chroot,
keyfile& keyfile) const
{
- keyfile::set_object_value(*this, &chroot_facet_personality::get_persona,
- keyfile, chroot.get_keyfile_name(), "personality");
+ // Only set if defined.
+ if (get_persona().get_name() != "undefined")
+ keyfile::set_object_value(*this, &chroot_facet_personality::get_persona,
+ keyfile, chroot.get_keyfile_name(), "personality");
}
void
diff --git a/sbuild/sbuild-personality.cc b/sbuild/sbuild-personality.cc
index 1954562..509b1d4 100644
--- a/sbuild/sbuild-personality.cc
+++ b/sbuild/sbuild-personality.cc
@@ -96,24 +96,17 @@ sbuild::personality::personalities(initial_personalities,
initial_personalities + (sizeof(initial_personalities) / sizeof(initial_personalities[0])));
sbuild::personality::personality ():
- persona(
-#if defined(SBUILD_FEATURE_PERSONALITY)
- ::personality(0xffffffff)
-#else
- 0xffffffff
-#endif // SBUILD_FEATURE_PERSONALITY
- )
-{
-}
-
-sbuild::personality::personality (type persona):
- persona(persona)
+ persona_name("undefined"),
+ persona(find_personality("undefined"))
{
+ set_name("undefined");
}
sbuild::personality::personality (std::string const& persona):
- persona(find_personality(persona))
+ persona_name("undefined"),
+ persona(find_personality("undefined"))
{
+ set_name(persona);
}
sbuild::personality::~personality ()
@@ -149,7 +142,25 @@ sbuild::personality::find_personality (type persona)
std::string const&
sbuild::personality::get_name () const
{
- return find_personality(this->persona);
+ return this->persona_name;
+}
+
+void
+sbuild::personality::set_name (std::string const& persona)
+{
+ this->persona_name = persona;
+ this->persona = find_personality(persona);
+
+ if (this->persona_name != "undefined" &&
+ this->persona == find_personality("undefined"))
+ {
+ this->persona_name = "undefined";
+ this->persona = find_personality("undefined");
+
+ personality::error e(persona, personality::BAD);
+ e.set_reason(personality::get_personalities());
+ throw e;
+ }
}
sbuild::personality::type
@@ -163,7 +174,7 @@ sbuild::personality::set () const
{
#ifdef SBUILD_FEATURE_PERSONALITY
/* Set the process execution domain using personality(2). */
- if (this->persona != 0xffffffff &&
+ if (this->persona != find_personality("undefined") &&
::personality (this->persona) < 0)
{
throw error(get_name(), SET, strerror(errno));
diff --git a/sbuild/sbuild-personality.h b/sbuild/sbuild-personality.h
index b9fac6f..416eb3f 100644
--- a/sbuild/sbuild-personality.h
+++ b/sbuild/sbuild-personality.h
@@ -65,13 +65,6 @@ namespace sbuild
*
* @param persona the persona to set.
*/
- personality (type persona);
-
- /**
- * The constructor.
- *
- * @param persona the persona to set.
- */
personality (std::string const& persona);
///* The destructor.
@@ -85,6 +78,14 @@ namespace sbuild
std::string const& get_name () const;
/**
+ * Set the name of the personality.
+ *
+ * @param persona the persona to set.
+ * @returns the personality name.
+ */
+ void set_name (std::string const& persona);
+
+ /**
* Get the personality.
*
* @returns the personality.
@@ -125,15 +126,7 @@ namespace sbuild
if (std::getline(stream, personality_name))
{
- rhs.persona = find_personality(personality_name);
-
- if (rhs.get_name() == "undefined" &&
- rhs.get_name() != personality_name)
- {
- personality::error e(personality_name, personality::BAD);
- e.set_reason(personality::get_personalities());
- throw e;
- }
+ rhs.set_name(personality_name);
}
return stream;
@@ -177,6 +170,9 @@ namespace sbuild
static std::string const&
find_personality (type persona);
+ /// The name of the current personality.
+ std::string persona_name;
+
/// The personality type.
type persona;
diff --git a/test/sbuild-personality.cc b/test/sbuild-personality.cc
index 3fe4f2a..9a1bf6c 100644
--- a/test/sbuild-personality.cc
+++ b/test/sbuild-personality.cc
@@ -29,9 +29,12 @@ class test_personality : public TestCase
{
CPPUNIT_TEST_SUITE(test_personality);
CPPUNIT_TEST(test_construction);
+ CPPUNIT_TEST_EXCEPTION(test_construction_fail, sbuild::personality::error);
CPPUNIT_TEST(test_output);
CPPUNIT_TEST(test_input);
CPPUNIT_TEST_EXCEPTION(test_input_fail, sbuild::personality::error);
+ CPPUNIT_TEST(test_set);
+ CPPUNIT_TEST_EXCEPTION(test_set_fail, sbuild::personality::error);
CPPUNIT_TEST_SUITE_END();
public:
@@ -45,19 +48,7 @@ public:
test_construction()
{
sbuild::personality p1;
-#if defined(SBUILD_FEATURE_PERSONALITY) && defined (__linux__)
- CPPUNIT_ASSERT(p1.get_name() == "linux" ||
- p1.get_name() == "linux_32bit" ||
- p1.get_name() == "linux32");
-#else
CPPUNIT_ASSERT(p1.get_name() == "undefined");
-#endif
-
- sbuild::personality p2(0xffffffff);
- CPPUNIT_ASSERT(p2.get_name() == "undefined");
-
- sbuild::personality p3("invalid_personality");
- CPPUNIT_ASSERT(p3.get_name() == "undefined");
sbuild::personality p4("linux");
#if defined(SBUILD_FEATURE_PERSONALITY) && defined (__linux__)
@@ -68,18 +59,14 @@ public:
}
void
- test_output()
+ test_construction_fail()
{
- sbuild::personality p2(0xffffffff);
- std::ostringstream ps2;
- ps2 << p2;
- CPPUNIT_ASSERT(ps2.str() == "undefined");
-
sbuild::personality p3("invalid_personality");
- std::ostringstream ps3;
- ps3 << p3;
- CPPUNIT_ASSERT(ps3.str() == "undefined");
+ }
+ void
+ test_output()
+ {
sbuild::personality p4("linux");
std::ostringstream ps4;
ps4 << p4;
@@ -118,7 +105,30 @@ public:
sbuild::personality p3;
std::istringstream ps3("invalid_personality");
ps3 >> p3;
- CPPUNIT_ASSERT(p3.get_name() == "undefined");
+ }
+
+ void
+ test_set()
+ {
+ sbuild::personality p2;
+ p2.set_name("undefined");
+ CPPUNIT_ASSERT(p2.get_name() == "undefined");
+
+ sbuild::personality p4;
+#if defined(SBUILD_FEATURE_PERSONALITY) && defined (__linux__)
+ p4.set_name("linux");
+ CPPUNIT_ASSERT(p4.get_name() == "linux");
+#else
+ p4.set_name("undefined");
+ CPPUNIT_ASSERT(p4.get_name() == "undefined");
+#endif
+ }
+
+ void
+ test_set_fail()
+ {
+ sbuild::personality p3;
+ p3.set_name("invalid_personality");
}
};
diff --git a/test/test-sbuild-chroot.h b/test/test-sbuild-chroot.h
index cd1abd1..cedb7c4 100644
--- a/test/test-sbuild-chroot.h
+++ b/test/test-sbuild-chroot.h
@@ -191,7 +191,6 @@ public:
keyfile.set_value(group, "root-groups", "group3,group4");
keyfile.set_value(group, "environment-filter",
SBUILD_DEFAULT_ENVIRONMENT_FILTER);
- keyfile.set_value(group, "personality", "undefined");
keyfile.set_value(group, "command-prefix", "");
keyfile.set_value(group, "script-config", "default/config");
}
signature.asc
Description: Digital signature
