|
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.