Follow @Openwall on Twitter for new release announcements and other news
[<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.