Skip Menu |
 

From: Mark H Weaver <mhw@netris.org>
To: krb5-bugs@mit.edu
Subject: [PATCH] Don't assume function arguments are evaluated right-to-left
Date: Thu, 07 Nov 2013 00:32:42 -0500
Download (untitled) / with headers
text/plain 1.4KiB
On mips64el-linux-gnu (using the MIPS N32 ABI), GCC 4.7.3 reports that
'context' is used before it is initialized in the first statement of
'main' in src/lib/krb5/krb/t_cc_config.c.

Indeed, the code assumes that function arguments will be evaluated in
right-to-left order. I see nothing in the relevant C standards that
would justify this assumption. C99 section 6.5.2.3 paragraph 10 states:

The order of evaluation of the function designator, the actual
arguments, and subexpressions within the actual arguments is
unspecified, but there is a sequence point before the actual call.

The same paragraph of C11 is worded differently, but amounts to the same
thing: "There is a sequence point after the evaluations of the function
designator and the actual arguments but before the actual call." There
is no mention of any other sequence points.

The following patch fixes this problem.

Regards,
Mark


--- src/lib/krb5/krb/t_cc_config.c.orig 2012-12-17 21:47:05.000000000 -0500
+++ src/lib/krb5/krb/t_cc_config.c 2013-10-29 18:19:16.547994590 -0400
@@ -117,8 +117,8 @@
int c;
unsigned int i;

- bail_on_err(context, "Error initializing Kerberos library",
- krb5_init_context(&context));
+ ret = krb5_init_context(&context);
+ bail_on_err(context, "Error initializing Kerberos library", ret);
bail_on_err(context, "Error getting location of default ccache",
krb5_cc_default(context, &ccache));
server = NULL;
This was fixed on master in commit
525eafc83a0fbe8f215b7749b5774d54468a19d1 (March 2013). Unless it is
causing a build failure, I don't think there is a need to backport the
fix to prior releases since the bug is in a test program.

I don't think any argument evaluation order would make the old code
correct; it was a straightforward case of use before initialization. But
it is true that C does not guarantee a particular order of evaluation for
function arguments.
From: Mark H Weaver <mhw@netris.org>
To: rt@krbdev.mit.edu
Subject: Re: [krbdev.mit.edu #7760] [PATCH] Don't assume function arguments are evaluated right-to-left
Date: Thu, 07 Nov 2013 18:57:01 -0500
RT-Send-Cc:
"Greg Hudson via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
> This was fixed on master in commit
> 525eafc83a0fbe8f215b7749b5774d54468a19d1 (March 2013).

If the problem was fixed in March, why is the bug still present in
1.11.4, released a few days ago?

Show quoted text
> Unless it is causing a build failure, I don't think there is a need to
> backport the fix to prior releases since the bug is in a test program.

It _does_ cause a build failure, because you pass options to GCC that
turn that warning into a fatal error.

Show quoted text
> I don't think any argument evaluation order would make the old code
> correct; it was a straightforward case of use before initialization.

Fair enough. My hope was that if 'krb5_init_context' reported an error,
it would have partially initialized 'context', enough for 'bail_on_err'
to work properly. Admittedly, I didn't not investigate whether this
hope was justified.

In any case, without my patch 1.11.4 fails to build at all on my system.
With the patch, it builds and passes the test suite.

Regards,
Mark
To: rt@krbdev.MIT.EDU
Subject: Re: [krbdev.mit.edu #7760] [PATCH] Don't assume function arguments are evaluated right-to-left
From: Tom Yu <tlyu@MIT.EDU>
Date: Fri, 08 Nov 2013 13:43:08 -0500
RT-Send-Cc:
Download (untitled) / with headers
text/plain 1.3KiB
"Mark H Weaver via RT" <rt-comment@krbdev.mit.edu> writes:

Show quoted text
> "Greg Hudson via RT" <rt-comment@krbdev.mit.edu> writes:
>
>> This was fixed on master in commit
>> 525eafc83a0fbe8f215b7749b5774d54468a19d1 (March 2013).
>
> If the problem was fixed in March, why is the bug still present in
> 1.11.4, released a few days ago?

We have a process for designating which commits should be applied to
prior release branches. This commit was not designated in that way,
but now that we are aware of the need for this patch, we can include
it in a subsequent krb5-1.11 release.

Show quoted text
>> Unless it is causing a build failure, I don't think there is a need to
>> backport the fix to prior releases since the bug is in a test program.
>
> It _does_ cause a build failure, because you pass options to GCC that
> turn that warning into a fatal error.

Thank you for letting us know about the build failure you experienced.
It appears that the bug does not cause a build failure on the
platforms where we regularly build krb5-1.11, so we did not consider
it a priority to apply the patch to the krb5-1.11 branch. Now that we
are aware of the build failure, we have a justification for applying
the patch.

Show quoted text
> In any case, without my patch 1.11.4 fails to build at all on my system.
> With the patch, it builds and passes the test suite.

Please let us know if the commit that Greg mentioned also fixes the
build for you.
From: tlyu@mit.edu
Subject: git commit

Fix use-before-init in two test programs

If krb5_init_context fails, use a null context for getting the error
message, not a context we haven't yet initialized. Observed by David
Benjamin <davidben@mit.edu> using clang.

(cherry picked from commit 525eafc83a0fbe8f215b7749b5774d54468a19d1)

https://github.com/krb5/krb5/commit/07704a74aca13a05d4959dc1c0d1b85889dbd7bf
Author: Greg Hudson <ghudson@mit.edu>
Committer: Tom Yu <tlyu@mit.edu>
Commit: 07704a74aca13a05d4959dc1c0d1b85889dbd7bf
Branch: krb5-1.11
src/lib/krb5/krb/t_cc_config.c | 2 +-
src/lib/krb5/krb/t_in_ccache.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)