On November 2, 2003 08:31 am, Ben Laurie wrote: > Geoff Thorpe wrote: > > - There are other things to fix/change - including making the > > existing macros and functions stop tolerating two representations of > > zero, this may highlight more crappy code elsewhere. But one thing at > > a time ... > > We don't care whether they tolerate them, do we? We just care that they > don't create them. I understand that making them intolerant is a > debugging aid, of course.
Yeah this is the point, the code is pretty pathological so I want to squeeze the rules a little tighter to see what bugs we can identify. As with my existing debugging macros, I'd probably continue to leave the existing "tolerant" behaviour and only switch it to retentive mode when BN_DEBUG is defined. For now at least, and perhaps for the next release (or two), the ideal would be to eliminate openssl code that generates "dodgy" bignum states but leave the tolerance of them there a bit longer for the purposes of external code. BN_DEBUG is one way to manage that, and in particular it gives developers of external code a mechanism to validate their own code too before the "intolerant" behaviour becomes the default at a later date. The reasons for making this the default eventually should be fairly obvious. The current code and the difficulty of this kind of "cleanup" is a product of letting ambiguous data pass through long code-paths and "sanitising" it (with bn_fix_top()) too far away from the source of the problem (typically in a different function). > > - I think I've got most of the function exit paths covered with > > bn_check_top() macros, though there may be those I've missed. > > Might be good to check function entry (at least in debug mode). This is already the case. All the bn_check_top()s and bn_fix_top()s that occured on function entry for a function's parameters are, or should be (please feel free to scrutinise the patch), converted into debugging checks when BN_DEBUG is defined. This is handled by deprecating bn_fix_top() for a new macro - bn_correct_top() - such that when in debugging mode, bn_fix_top() is a debugging check, and when not in debugging mode, it maps directly to bn_correct_top(). So, bn_fix_top()s are only converted to bn_correct_top() once they trip up an alert() and the code is subsequently verified to be an appropriate use of the macro (ie. you're correcting 'top' after directly manipuating the bignum). The principle is to never tolerate the need to bn_correct_top() input parameters, and the stack-trace from the corresponding alert() tells you where the 'bad' bignum is coming from. BTW: with BN_DEBUG_RAND defined, these function entry checks also introduce random noise in the words beyond bn->top to try and trip up lazy assumptions later. Unfortunately, it's already tripping up bug(s) that are *hard* to fix. :-) > > - It would be easiest to put this stuff in to CVS, as it will make > > collaboration easier and may give some people less reason to > > ignore it. Any objections? > > Go for it. OK, thanks for the vote. I will sit on this for another day or so perhaps, as I want to go in and go through it all again to double-check what state it is in (you saw the flurry of commits, I have a few checkouts to consolidate because of them :-). I guess the thing to do after committing would be to draw up a list of "tasks" that interested parties could help me with. Eg. running on different processors, different debugging levels, configuration targets, using valgrind, etc etc. I'd be tempted to move discussion of this sort of thing to the request tracker, but the lack of threading there makes it a bit terrifying (if people try to help, we'll quickly end up with a gigantic unmanagable ticket). Any thoughts about that would be welcome too. Cheers, Geoff -- Geoff Thorpe [EMAIL PROTECTED] http://www.geoffthorpe.net/ ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [EMAIL PROTECTED] Automated List Manager [EMAIL PROTECTED]
