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.