MD5 compatibility fixes (was: [SCM] Samba Shared Repository - branch master updated)
jelmer at samba.org
Sat Dec 8 08:27:33 MST 2012
On Sat, Dec 08, 2012 at 05:17:17PM +0200, Alexander Bokovoy wrote:
> On Sat, Dec 8, 2012 at 5:12 PM, Jelmer Vernooij <jelmer at samba.org> wrote:
> > On Sat, Dec 08, 2012 at 04:55:23PM +0200, Alexander Bokovoy wrote:
> >> On Sat, Dec 8, 2012 at 2:44 PM, Jelmer Vernooij <jelmer at samba.org> wrote:
> >> > On Sat, Dec 08, 2012 at 01:31:03PM +0100, Andreas Schneider wrote:
> >> >> Changeset truncated at 500 lines:
> >> >>
> >> >> diff --git a/buildtools/wafsamba/samba_utils.py b/buildtools/wafsamba/samba_utils.py
> >> >> index c1869df..cab87a4 100644
> >> >> --- a/buildtools/wafsamba/samba_utils.py
> >> >> +++ b/buildtools/wafsamba/samba_utils.py
> >> >> @@ -388,9 +388,17 @@ def RUN_COMMAND(cmd,
> >> >> # make sure we have md5. some systems don't have it
> >> >> try:
> >> >> from hashlib import md5
> >> >> + try:
> >> >> + foo = md5.md5('abcd')
> >> >> + except ValueError:
> >> >> + raise
> >> > This looks really odd - what does the 'except ValueError: ; raise' bit do?
> >> > Wouldn't the behaviour of this code be the same without it?
> >> No, it is not.
> >> FIPS mode is preventing certain (outdated) cryptographic operations to
> >> be used. When operating system runs in FIPS mode all access to actual
> >> banned crypto algorithms will cause failures.
> >> In FIPS mode Python md5 module can be imported without any issues and
> >> ValueError exception will be raised when actually attempting to call
> >> to md5 function. Thus, we would never see that the code would actually
> >> be unusable with existing check.
> >> This leads to sudden failure of configure stage later because waf uses
> >> hash function (md5 by default) to notice differences between objects'
> >> states. The hash calculation is done deep in waf code causes unhandled
> >> exception in FIPS mode and that one causes early exit from
> >> run_c_code() routine, leaving unbalanced "enter directory - leave
> >> directory" pairs. The latter causes sudden removal of the temporary
> >> directory while it is in use by the configure check which, in turn,
> >> causes os.getcwd('.') to throw exception (since current directory was
> >> just deleted) and abort the execution.
> > That bit seems reasonable; I'm curious about the re-raising of the
> > exception.
> >> The way I've coded this fix we are re-using existing infra in wafsamba
> >> to replace hash function. I'm re-raising exception that FIPS mode may
> >> raise so that we are falling through to actual replacement code.
> > What is the point of re-raising the exception? This:
> > try:
> > x = md5.md5('foo')
> > except ValueError:
> > raise
> > should be equivalent to this:
> > x = md5.md5('foo')
> > If not, can you explain what the difference is?
> The latter will trigger for all kinds of exceptions while the former
> is only covering ValueError ones.
But then you just reraise the exception, which results in the same behaviour
as the default behaviour for exceptions that aren't caught.
ValueError will be treated exactly same way as KeyError, ImportError, etc.
and just be propagated up.
> Perhaps you are right in that if we ever trigger any exception
> accessing md5.md5() we would need to force replacement route
> unconditionally. I wanted to codify explicit error so that we don't
> forget why we are doing all this.
A comment explaining what we are looking for and why would be better
in that case. I at least didn't understand the intent of this code
(hence my mail :-).
More information about the samba-technical