Bug 37701

Summary:	telepathy-salut-0.5.0 build failure: error: 'attr' may be used uninitialized in this function [-Wuninitialized]
Product:	Wocky	Reporter:	Ed Catmur <ed>
Component:	General	Assignee:	Telepathy bugs list <telepathy-bugs>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	CC:	freedesktop-bugs, will
Version:	unspecified
Hardware:	Other
OS:	All
Whiteboard:
i915 platform:		i915 features:
Attachments:	wocky-uninitialized.patch SCRAM: correct logic in server-final message handling

Description Ed Catmur 2011-05-28 14:31:48 UTC

Downstream: http://bugs.gentoo.org/show_bug.cgi?id=369091

/bin/sh ../libtool  --tag=CC   --mode=compile i686-pc-linux-gnu-gcc -std=gnu99
-DHAVE_CONFIG_H -I. -I..    -Wall -Wextra -Wdeclaration-after-statement
-Wshadow -Wstrict-prototypes -Wmissing-prototypes -Wsign-compare
-Wnested-externs -Wpointer-arith -Wformat-security -Winit-self
-Wno-missing-field-initializers -Wno-unused-parameter -Werror
-Wno-error=missing-field-initializers -Wno-error=unused-parameter  -pthread
-I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include   -I/usr/include/libxml2
-DG_LOG_DOMAIN=\"wocky\" -ggdb -O1 -O2 -O3 -pipe -march=core2 -mmmx -msse
-m3dnow -fdiagnostics-show-option -MT wocky-sasl-scram.lo -MD -MP -MF
.deps/wocky-sasl-scram.Tpo -c -o wocky-sasl-scram.lo wocky-sasl-scram.c
libtool: compile:  i686-pc-linux-gnu-gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I..
-Wall -Wextra -Wdeclaration-after-statement -Wshadow -Wstrict-prototypes
-Wmissing-prototypes -Wsign-compare -Wnested-externs -Wpointer-arith
-Wformat-security -Winit-self -Wno-missing-field-initializers
-Wno-unused-parameter -Werror -Wno-error=missing-field-initializers
-Wno-error=unused-parameter -pthread -I/usr/include/glib-2.0
-I/usr/lib/glib-2.0/include -I/usr/include/libxml2 -DG_LOG_DOMAIN=\"wocky\"
-ggdb -O1 -O2 -O3 -pipe -march=core2 -mmmx -msse -m3dnow
-fdiagnostics-show-option -MT wocky-sasl-scram.lo -MD -MP -MF
.deps/wocky-sasl-scram.Tpo -c wocky-sasl-scram.c  -fPIC -DPIC -o
.libs/wocky-sasl-scram.o
cc1: warnings being treated as errors
wocky-sasl-scram.c: In function 'scram_handle_auth_data':
wocky-sasl-scram.c:528:60: error: 'attr' may be used uninitialized in this
function [-Wuninitialized]
wocky-sasl-scram.c:526:9: note: 'attr' was declared here
make[8]: *** [wocky-sasl-scram.lo] Error 1

I believe -Werror is being added to CFLAGS by the package; my CFLAGS are

CFLAGS="-ggdb -O1 -O2 -O3 -pipe -march=core2 -mmmx -msse -m3dnow
-fdiagnostics-show-option"

OK, that's confusing: the subdir lib/ext/wocky has its own autotools setup
which appears to have been shipped in a development state:

m4_define([wocky_nano_version], [1])
...
TP_COMPILER_WARNINGS([ERROR_CFLAGS], [test "x$official_release" = xno],

Comment 1 Ed Catmur 2011-05-28 14:34:53 UTC

Created attachment 47259 [details] [review]
wocky-uninitialized.patch

Fix uninitialised variable.

Might I also request that you bump the minor on wocky or otherwise disable -Werror by default on release tarballs?

Comment 2 Will Thompson 2012-07-23 14:14:04 UTC

Thanks for the report! Actually, the correct fix is not to initialize the variable, but to change the conditional to use || not &&. Patch with explanation to follow, and reassigning to Wocky.

I believe that, these days, Wocky as bundled with Salut no longer sets -Werror.

Comment 3 Will Thompson 2012-07-23 14:15:26 UTC

Created attachment 64543 [details] [review]
SCRAM: correct logic in server-final message handling

The server-final message looks like this:

   server-final-message = (server-error / verifier)
                        ["," extensions]
   server-error = "e=" server-error-value
   verifier        = "v=" base64

The code was trying to check “is there at least one attribute, and is it
a verifier?”. But instead it was checking “is there at least one
attribute, and if not, is the non-existant attribute a verifier?” by
comparing the uninitialized 'attr' variable to 'v'.

I've checked other calls to scram_get_next_attr_value() and they seem to
get the logic the right way round.

This bug does not cause a security vulnerability. If the uninitialized
'attr' variable happens to contain the character 'v', then the following
call to scram_check_server_verification() will compare the contents of
value (which ought to be a verification string, but is NULL) to the
verification string as calculated by Wocky (which is not NULL) and so
Wocky will abort the connection.

Thanks to Ed Catmur for reporting the use of an uninitialized variable
on <https://bugs.freedesktop.org/show_bug.cgi?id=37701>.

Comment 4 Jonny Lamb 2012-07-23 14:22:36 UTC

Comment on attachment 64543 [details] [review]
SCRAM: correct logic in server-final message handling

Review of attachment 64543 [details] [review]:
-----------------------------------------------------------------

""#             m          
   #     mmmm  mm#mm  mmmmm 
   #    #" "#    #    # # # 
   #    #   #    #    # # # 
   "mm  "#m"#    "mm  # # # 
         m  #               
          ""

Comment 5 Will Thompson 2012-07-23 14:37:10 UTC

I've merged this to Wocky master. I'll leave it to someone else to update the Wocky snapshots in Salut and Gabble.

Comment 6 Will Thompson 2012-11-12 14:41:38 UTC

The fix will be in Salut 0.9.0 whenever that is released!

http://cgit.freedesktop.org/telepathy/telepathy-salut/commit/?id=4159cf378f26e8cd17d875667c68a3446e084acd

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.