[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190407161842.GA16539@openwall.com>
Date: Sun, 7 Apr 2019 18:18:42 +0200
From: Solar Designer <solar@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: ldr_split_line() performance regression
On Wed, Apr 03, 2019 at 11:47:35PM +0300, Aleksey Cherepanov wrote:
> Solar Designer <solar@...nwall.com> writes:
> > On Wed, Sep 16, 2015 at 02:02:34PM -0500, jfoug wrote:
> >>
> >> On 9/16/2015 1:52 PM, Solar Designer wrote:
> >> >strlen(*ciphertext) < 10 && strncmp(*ciphertext, "$dummy$", 7)) {
> >>
> >> These should be reversed, since strncmp should short circuit out much
> >> earlier than the length check.
I now think Jim was wrong about that: we're checking for _not_ matching
"$dummy$", and that condition will usually be true, so with these checks
reversed the strlen() will be reached almost all of the time anyway.
> >> Also, checking for a '$' char even
> >> before a strncpy (or even '$' and 'd') would be smart,
> >
> > Good catch. I suggest we do:
> >
> > if (((*login)[0] == '+' && (!(*login)[1] || (*login)[1] == '@')) &&
> > (*ciphertext)[0] != '$' &&
> > strlen(*ciphertext) < 10 && strncmp(*ciphertext, "$dummy$", 7)) {
>
> If (*ciphertext)[0] != '$' condition is true, then
> strncmp(*ciphertext, "$dummy$", 7) != 0 condition is true too.
>
> So it seems there was a mistake in evolution of the check.
Yeah, this looks broken to me. The change got in as:
/* Check for NIS stuff */
if (((*login)[0] == '+' && (!(*login)[1] || (*login)[1] == '@')) &&
(*ciphertext)[0] != '$' && strnlen(*ciphertext, 10) < 10 &&
strncmp(*ciphertext, "$dummy$", 7)) {
(which is similar to what I wrongly wrote on john-dev), whereas it
should probably have been:
/* Check for NIS stuff */
if (((*login)[0] == '+' && (!(*login)[1] || (*login)[1] == '@')) &&
strnlen(*ciphertext, 10) < 10 &&
((*ciphertext)[0] != '$' || strncmp(*ciphertext, "$dummy$", 7))) {
Also, since the checks of *ciphertext are only reached in the rare
special case of the login name looking NIS'ish, we might not have needed
to optimize them at all.
In core, the code is:
/* Check for NIS stuff */
if (((*login)[0] == '+' && (!(*login)[1] || (*login)[1] == '@')) &&
strlen(*ciphertext) < 10 && strncmp(*ciphertext, "$dummy$", 7))
return 0;
So we should probably just revert jumbo's to that. I don't mind keeping
magnum's use of strnlen(), though, since jumbo has a dependency on that
function in other places anyway (core doesn't).
magnum, will you take care of this, please?
I should have given this more thought before ack'ing Jim's suggestion.
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.
