[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20120117090259.GA22205@openwall.com>
Date: Tue, 17 Jan 2012 13:02:59 +0400
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: Recent CVS patches
On Tue, Jan 17, 2012 at 12:07:39AM +0100, magnum wrote:
> On 01/16/2012 08:47 PM, Solar Designer wrote:
> > You seem to be right. The alignment requirements in the self-tests in
> > the current CVS tree are too strict.
> >
> > How do you suggest we deal with this? Introduce FMT_* flags that tell
> > the self-tests that misalignment of binary and/or salt is OK? Or simply
> > drop this test for all?
>
> It would be a pity dropping that test. Especially since most of us use
> intel. Actually I think it's easier to make a format return aligned data
> (needed or not), than to add the flag (because we'd need to take care
> that such a flag is proper). From a quick glance there were just six
> formats that failed this, four binary() and two salt(). I can fix them
> instead, even if not really needed for those formats.
What are these six?
I am concerned that there may be more - just not detected in your
specific build because the returned addresses just happened to be
aligned. The self-tests currently pass the pointer returned by split()
into binary() and salt(). If that pointer happens to be aligned, then a
binary() or/and salt() merely returning the same pointer won't be
detected as an error, but this may fail in another build. Perhaps we
can deliberately misalign the pointer passed into binary() and salt() by
the self-tests (we'll have to copy the string returned by split() for
that). But there can be other cases of binary() or/and salt() happening
to return an aligned pointer while having no guarantee that it will do
so in another build. Some of these would be problematic on most non-x86
architectures anyway, but as you correctly point out some are non-issues
everywhere. So as it is, this test potentially turns some non-issues
into issues that will unnecessarily show up on some builds.
Thus, I think the test needs to be disabled by default, maybe put within
an #ifdef DEBUG block that we'd only re-enable for our own testing, but
not for releases intended for end-users.
BTW, the same DEBUG could also enable other tests (not only in the
self-tests code in formats.c) that are unsuitable for production builds.
Currently, there are a few assert()'s in JtR source code, but none of
them have any significant drawbacks - so they're OK to have in
production builds. We could use #ifdef DEBUG ... #endif to wrap
possible additional assert()'s e.g. in performance-critical code paths,
where I did not dare to add assert()'s so far. DEBUG builds could also
print debugging information on their own even when all tests pass.
In fact, perhaps DEBUG must print at least one line (saying that it's
enabled) such that we don't inadvertently leave it enabled in a
production build or in a build where we rely on benchmark results.
And yes, I am aware of NDEBUG - the standard way to disable all
assert()'s for production builds - but I actually like keeping the
harmless ones of the assert()'s even in typical production builds, just
like we keep the self-tests there. So a second setting may be needed.
How does this sound to you?
Alexander
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
