On Wed, 2011-12-28 at 15:28:45 -0800, Kees Cook wrote:
> On Sun, Dec 18, 2011 at 09:42:50AM +0100, Guillem Jover wrote:
> > On Fri, 2011-12-16 at 16:39:25 -0800, Kees Cook wrote:
> > > Fresh patch attached! :)
> >
> > Thanks! Could you split the refactoring/cleaning into its own patch
> > (actually something that already crossed my mind when first seeing
> > the original buildflags code), and the new functionality into another
> > one?
>
> Sure! Hopefully I chose the right line for this split. I also split out the
> documentation adjustment that was unrelated to these changes.
Heh, I guess I should have been a bit more explicit. See below.
> Thanks for the review! New patches attached...
Thanks for your perseverance!
> From d3f7d8c41bca70887c4e6a24ec8736a36b9da828 Mon Sep 17 00:00:00 2001
> From: Kees Cook <k...@outflux.net>
> Date: Wed, 28 Dec 2011 15:22:55 -0800
> Subject: [PATCH 1/3] docs: clarify the relationship between relro/bindnow
[...]
> Signed-off-by: Kees Cook <k...@debian.org>
I guess you might want the From (Author) to match the Signed-off-by?
Otherwise, it looks good.
> From 1d208333d42fc1606542d27346c16ad1b71b3de4 Mon Sep 17 00:00:00 2001
> From: Kees Cook <k...@outflux.net>
> Date: Wed, 28 Dec 2011 15:03:44 -0800
> Subject: [PATCH 2/3] BuildFlags: Create feature interface
>
> Refactor the hardened compiler flag logic to use a new "get/set/has
> features" interface to BuildFlags.pm so as to communicate the current
> logical state of the hardening feature.
>
> Signed-off-by: Kees Cook <k...@debian.org>
> ---
> scripts/Dpkg/BuildFlags.pm | 40 ++++++++++++++++++++++++-
> scripts/Dpkg/Vendor/Debian.pm | 66 ++++++++++++++++++++++++++++------------
> 2 files changed, 85 insertions(+), 21 deletions(-)
>
> diff --git a/scripts/Dpkg/BuildFlags.pm b/scripts/Dpkg/BuildFlags.pm
> index 6112a9f..e832b39 100644
> --- a/scripts/Dpkg/BuildFlags.pm
> +++ b/scripts/Dpkg/BuildFlags.pm
I'd move changes to this file and...
> Returns a boolean indicating whether the flags exists in the object.
> diff --git a/scripts/Dpkg/Vendor/Debian.pm b/scripts/Dpkg/Vendor/Debian.pm
> index e824d0e..07935d9 100644
> --- a/scripts/Dpkg/Vendor/Debian.pm
> +++ b/scripts/Dpkg/Vendor/Debian.pm
> + # Store the feature usage.
> + for (keys %use_feature) {
> + $flags->set_feature("hardening", $_, $use_feature{$_})
> + }
> }
...this change to patch 3.
The rest of the patch looks good.
> From c92970f2d45d3fc15a263a7125b63b18a696ae42 Mon Sep 17 00:00:00 2001
> From: Kees Cook <k...@outflux.net>
> Date: Thu, 8 Dec 2011 15:53:14 -0800
> Subject: [PATCH 3/3] dpkg-buildflags: provide feature query ability
>
> Since the logic for having a hardening flag enabled or disabled depends
> on the architecture, and since the flags may change over time for each
> hardening feature, there needs to be a way to externally query the state
> of the hardening features. Specifically, lintian needs this to be able
> to figure out if a binary package is missing expected hardening features.
> Instead of maintaining multiple hard-coded lists of expected hardening
> features, this makes dpkg-buildflags the canonical location of the
> information, which can be queried by externally. (See bug 650536.)
>
> Signed-off-by: Kees Cook <k...@debian.org>
> ---
> man/dpkg-buildflags.1 | 16 ++++++++++++++++
> scripts/dpkg-buildflags.pl | 13 ++++++++++++-
> 2 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/scripts/dpkg-buildflags.pl b/scripts/dpkg-buildflags.pl
> index ee33961..f273ca1 100755
> --- a/scripts/dpkg-buildflags.pl
> +++ b/scripts/dpkg-buildflags.pl
[...]
> @@ -115,6 +117,15 @@ if ($action eq "get") {
> print $build_flags->get_origin($param) . "\n";
> exit(0);
> }
> +} elsif ($action eq "query-features") {
> + if ($build_flags->has_features($param)) {
> + my %features = $build_flags->get_features($param);
> + foreach my $feature (sort keys %features) {
> + print "Feature: $feature\nEnabled: ";
> + print $features{$feature} ? "yes\n\n" : "no\n\n";
> + }
> + exit(0);
> + }
I meant for example something like:
my $para_shown = 0;
foreach ... {
if ($para_shown) {
print "\n";
} else {
$para_shown = 1;
}
printf "Feature: %s\n", $feature;
printf "Enabled: %s\n", $features{$feature} ? "yes" : "no";
}
thanks,
guillem
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
