Date: Thu, 9 Jul 2009 21:51:50 -0500 From: "JimF" <jfoug@....net> To: <john-users@...ts.openwall.com> Subject: Re: patch for new john format: phpass (also works for phpBBv3) ----- Original Message ----- From: "Solar Designer" <solar@...nwall.com> To: <john-users@...ts.openwall.com> Sent: Thursday, July 09, 2009 8:38 PM Subject: Re: [john-users] patch for new john format: phpass (also works for phpBBv3) > No, I was referring to lines like: > > + sixbits = (unsigned)strchr(itoa64, *pos++); > + sixbits -= (unsigned)itoa64; > > Here you unnecessarily get into the dark area of casting pointers to > integers (even to integers of different size on some architectures). > A more correct way to write this would be: > > sixbits = strchr(itoa64, *pos++) - itoa64; > > that is, doing pointer math, which is well-defined (the result is of > signed integer type known as ptrdiff_t on modern systems). This assumes > that the string had been pre-validated, such that strchr() wouldn't > return NULL (I think you do have such pre-validation in place). > > Of course, changing to atoi64[ARCH_INDEX(...)] is an even better fix. I see that now, and it is part of the phpass-3.diff. One of the first things I did, was to build the phpassmd5_binary() function. It was not well done, but 'worked' for me. I put a comment there, because I also did not like it. I did not know how to use atoi64 at that time, and saw examples 'similar' to what I had done, in some other _fmt.c files, so went with that, but knew it was bad. However, looking for a bug in the wrong place, did show some other issues (in the non-mmx code within the get_hash_x() functions. Now those functions are much simpler, with no conditional compilation blocks. The phpassmd5_binary is now done with atoi64[ARCH_INDEX()] and it is much simpler, cleaner and easier to follow. And yes, I do see the portablity issues in the original code. Worked on my box, but might have had problems in other places. >> I now have a phpass-3.diff (but I need to test some). But as we talked >> about before (off list), I will look at getting this out in the bigger >> set of changes I have. > > No, we have not agreed about anything like that. I merely asked you to > post to the list about your bigger set of changes. I did not mean to infer the changes were going to be the way it is, just that I was going to post, and put them out. I have some additional clean up to do, then get them into a couple diffs, so that they will 'intermix' and properly patch. >For now, I'd prefer > you to release your phpass-3.diff stuff separately, if you don't mind. > You can also have it as part of your bigger patch, if you need that in > order to take advantage of other changes that you're making. -- To unsubscribe, e-mail john-users-unsubscribe@...ts.openwall.com and reply to the automated confirmation request that will be sent to you.
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.