Bug 37701 - telepathy-salut-0.5.0 build failure: error: 'attr' may be used uninitialized in this function [-Wuninitialized]
Summary: telepathy-salut-0.5.0 build failure: error: 'attr' may be used uninitialized ...
Status: RESOLVED FIXED
Alias: None
Product: Wocky
Classification: Unclassified
Component: General (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-28 14:31 UTC by Ed Catmur
Modified: 2012-11-12 14:41 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Attachments
wocky-uninitialized.patch (496 bytes, patch)
2011-05-28 14:34 UTC, Ed Catmur 		Details | Splinter Review
SCRAM: correct logic in server-final message handling (1.98 KB, patch)
2012-07-23 14:15 UTC, Will Thompson 		Details | Splinter Review
View All

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.