Wireshark-bugs: [Wireshark-bugs] [Bug 2162] for() loop evaluation order in ep_strndup() can trig

Date: Fri, 4 Jan 2008 19:08:30 +0000 (GMT)
http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=2162





------- Comment #6 from jyoung@xxxxxxx  2008-01-04 19:08 GMT -------
Hello Jeff,

Is there another bug?  

Perhaps, but it may be more a question of conservative/safe use versus
liberal/risky use of a length bounded string functions with a not necessarily
NUL terminated character array.   

The valgrind reported ep_strndup() bug was triggered when the uat_load.l
derived code was processing the "auth_model=" and the "priv_proto=" tokens of
the snmp_users file. These are the 3rd and 5th fields a snmp_users config
record.   

Here's an example of what an snmp_users config record "might" look like:

> jyoung@jhy-x2002:~/projects/wireshark> cat ~/.wireshark/snmp_users
> # This file is automatically generated, DO NOT MODIFY.
> aabbcc,"user1","MD5","myauthpassword","DES","myprivpassword"
> jyoung@jhy-x2002:~/projects/wireshark>

The 3rd and 5th fields (the auth_model= and priv_proto= respectively)
apparently have a set of fixed values.  I didn't chase down the place where the
the actual 'MD5' and 'DES' character arrays that uat->str eventually point to
were copied/created from.  Although I THINK this is somehow related to the use
of UAT_VS_DEF() macro in asn1/snmp/packet-snmp-template.c 

I got redirected when my instrumentation code (a printf("%s") of the uat->str
value) triggered an identical valgrind "Invalid read of size 1" error.

<snip>
> UAT_DEBUG: uat_fld_chk_enum(): uat.c:386: u1 = 0xacc6e98
> ==27451== Invalid read of size 1
> ==27451==    at 0x401D481: strlen (mc_replace_strmem.c:242)
> ==27451==    by 0x64C43D7: vfprintf (in /lib/tls/libc-2.3.5.so)
> ==27451==    by 0x64C98A2: printf (in /lib/tls/libc-2.3.5.so)
> ==27451==    by 0x4497541: uat_fld_chk_enum (uat.c:387)
> ==27451==    by 0x44A348F: uat_load_lex (uat_load.l:177)
> ==27451==    by 0x44A4A22: uat_load (uat_load.l:269)
> ==27451==    by 0x449728C: uat_load_all (uat.c:317)
> ==27451==    by 0x446AA97: init_prefs (prefs.c:1080)
> ==27451==    by 0x446AFAF: read_prefs (prefs.c:1245)
> ==27451==    by 0x8087B69: main (main.c:2506)
<snip>

It was then that I realized that I was assuming that uat->str was a (proper)
string.  The variable name sure gives one that impression! ;-)

Now here's the funny part about my debug printf()s.   There apparently was an
ASCII NUL byte in memory following each of the 'MD5' and 'DES' character
arrays.  So the printf("%s", uat->str) actually displayed the expected value. 
It appears from valgrind's point-of-view, the terminating NUL character was not
formally a part of the array and hence triggered the error message.  

Without fully appreciating the UAT_VS_DEF() macro "magic" I suspect that 'MD5'
and 'DES' are created as three byte array's in the module's global space during
the compilation of the epan/dissectors/packet-snmp.c.  Simple compilation
alignment issues would most likely result in padding the trailing end of these
three byte arrays with what appears to be ASCII NULs.  In this case we end up
with what appears to be a proper string but which valgrind (properly) reports
as a "invalid read of size 1" when one attempts to read the ASCII NUL.

So is there another bug?  Perhaps.  ;-)


-- 
Configure bugmail: http://bugs.wireshark.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.