[buildd-tools-devel] Bug#580136: Bug#580136: schroot personality build fails on armel
Roger Leigh
rleigh at codelibre.net
Sun Jun 6 11:31:37 UTC 2010
tags 580136 + fixed-upstream pending
thanks
On Sat, May 15, 2010 at 09:57:25PM +0100, Roger Leigh wrote:
> tags 580136 + patch
> thanks
>
> On Fri, May 07, 2010 at 05:04:51PM +0200, Arnaud Patard wrote:
> > Roger Leigh <rleigh at 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");
> }
> _______________________________________________
> Buildd-tools-devel mailing list
> Buildd-tools-devel at lists.alioth.debian.org
> http://lists.alioth.debian.org/mailman/listinfo/buildd-tools-devel
--
.''`. 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20100606/e8194c12/attachment-0002.pgp>
More information about the Buildd-tools-devel
mailing list