On Tue, May 22, 2012 at 12:06:38AM +0100, Mark Brown wrote: > On Mon, May 21, 2012 at 08:51:16PM +0200, Simon Ruderich wrote: >> On Mon, May 21, 2012 at 08:42:11AM +0100, Mark Brown wrote: >> >> And fixing the upstream build system to respect flags from the >> environment sounds like a good idea to me. > > That's nice but it's really not the sort of thing you should be doing as > part of something that's supposedly urgent
True, but I thought I'd fix the build system while I was already
at it fixing the hardening flags ...
> To repeat:
>
> - Provide patches in a more sensible format, sending a single message
> containing independant inline and attached patches makes things
> needlessly complicated to apply.
You're right, sorry. I'll use one type of patch in a bug report
in the future.
> - Don't make invasive changes to upstream when you don't have to,
> otherwise you're just making work for the package maintainers.
I still don't understand why changes to 10 lines count as
invasive. They were minimal and fixed the problem (something
which wasn't possible without the patch). But I'll try to reduce
the size of the changes in future patches.
> I'd also add (which wasn't mentioned before) that you should actually
> describe what the problem you're reporting is directly, just providing a
> vauge description and sending a large patch doesn't do this. You just
> said that the build system was "ignoring some hardening flags" but
> didn't mention which or why.
I thought that was pretty obvious. The links I provided introduce
the concept/ideas behind the hardening flags, the reason why some
may be missing and my report mentioned that some were missing in
debian/rules and in the build system.
It's true that I didn't provide exact details which flags were
missing, but I have reported many of these bugs and duplicating
the information in the patch and the report just takes too much
time (and doesn't provide any additional insights).
> This all made your report much harder to work with than it needed to be,
> the overall impression was "just apply this stuff, you don't need to
> understand it".
That wasn't my intention. I provided the background links to
clarify questions about hardening.
> Fix the actual problem. Don't introduce random stylistic changes as
> part of an "urgent" fix, and the more invasive your changes are the
> clearer you need to be about why you're making them.
That's what I tried - and the solution you've applied to zlib
does exactly the same as my original patch, it patches
contrib/minizip/Makefile (I wasn't sure that the binaries created
in Makefile.in were only example files so I patched it as well).
So I don't understand your criticism about my patch.
> I also had to reverse engineer what the problem you were trying to
> report was - your patch was not only your proposed fix, it was also your
> description of the problem.
See above. I admit it's not perfect, but the patch was simple and
I thought mentioning that the flags were missing, and where, is
enough - in addition to the patch.
> It is important that you report problems clearly, by providing your
> report in the form that you did what you were saying was that it's
> critical that we rewrite to your taste. If you want to do stuff like
> that that's fine but you should be separating it out, and ideally do it
> directly upstream since it's clearly not the sort of change we should be
> making in packages if we don't have to.
I only wanted to provide a solution with causes minimal work for
me and you as maintainers (didn't work so well in this case).
>> However the following command should help, it displays missing
>> flags of all binaries and libraries in the current directory and
>> ignores flags not enabled by default:
>>
>> find -type f \( -executable -o -name \*.so\* \) -exec hardening-check
>> --quiet --nopie --nobindnow {} + 2>/dev/null | grep -vF '(ignored)'
>
> That doesn't error out if hardening-check isn't installed which really
Interesting, I didn't know that. I thought find would print an
error.
> isn't what you want and franly how one is supposed to guess that those
> options are appropriate is beyond me...
They are described in the hardening links I added to my original
patch. But you're right of course, hardening-check should disable
the check for them by default (and provide a way to check all
executables). I'll work on a patch when I have some time.
Anyway, thanks for applying the patch.
Regards,
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
pgpwetPcmJze6.pgp
Description: PGP signature
