Opened 3 weeks ago

Last modified 24 hours ago

#30996 needs_information defect

namemap_get_or_create_id reads past its allocated memory

Reported by: arma Owned by: nickm
Priority: High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 041-must 041-regression
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

running valgrind on moria1, running git master

Jun 26 15:48:27.309 [notice] Tor 0.4.2.0-alpha-dev (git-6afe1b00c9c73b1b) running on Linux with Libevent 2.2.0-alpha-dev, OpenSSL 1.0.1e-fips, Zlib 1.2.3, Liblzma N/A, and Libzstd N/A.

I get at startup a pile of these:

==48499== Invalid read of size 4
==48499==    at 0x30E83A: namemap_get_or_create_id (namemap.c:29)
==48499==    by 0x301792: get_subsys_id (dispatch_naming.c:62)
==48499==    by 0x169D75: subsystems_add_pubsub_upto (subsysmgr.c:131)
==48499==    by 0x168A95: tor_run_main (main.c:1239)
==48499==    by 0x165D52: tor_main (tor_api.c:164)
==48499==    by 0x1659D8: main (tor_main.c:32)
==48499==  Address 0x72bdc40 is 32 bytes inside a block of size 33 alloc'd
==48499==    at 0x4A06A2E: malloc (vg_replace_malloc.c:270)
==48499==    by 0x322397: tor_malloc_ (malloc.c:45)
==48499==    by 0x322685: tor_malloc_zero_ (malloc.c:71)
==48499==    by 0x30E7F6: namemap_get_or_create_id (namemap.c:147)
==48499==    by 0x301792: get_subsys_id (dispatch_naming.c:62)
==48499==    by 0x169D75: subsystems_add_pubsub_upto (subsysmgr.c:131)
==48499==    by 0x168A95: tor_run_main (main.c:1239)
==48499==    by 0x165D52: tor_main (tor_api.c:164)
==48499==    by 0x1659D8: main (tor_main.c:32) 

Looks like it's allocating some space, and then trying to use more than it allocated?

Child Tickets

Change History (10)

comment:1 Changed 3 weeks ago by nickm

Keywords: 041-must added
Milestone: Tor: 0.4.1.x-final
Owner: set to nickm
Points: 0.5
Priority: MediumHigh
Status: newaccepted

comment:2 Changed 3 weeks ago by nickm

Keywords: 041-regression added

comment:3 Changed 3 weeks ago by nickm

"32 bytes inside a block of size 33" sounds legal to me... but a "read of size 4" here would of course not be illegal.

I note that line 29 is:

  return (unsigned) siphash24g(a->name, strlen(a->name));

So unless it's actually complaining about siphash24g, it's complaining about strlen(). I wonder if there is an issue with an optimized strlen you have? Sometimes valgrind doesn't understand those. For example see https://bugzilla.redhat.com/show_bug.cgi?id=518247 .

comment:4 Changed 3 weeks ago by arma

Yuck. I see the valgrind complaint with -O2 but I do not see it with -O0 or -O1. So I think you are right.

I have now added

diff --git a/src/lib/container/namemap.c b/src/lib/container/namemap.c
index a90057b..263e823 100644
--- a/src/lib/container/namemap.c
+++ b/src/lib/container/namemap.c
@@ -145,7 +145,10 @@ namemap_get_or_create_id(namemap_t *map,
     return NAMEMAP_ERR; /* Can't allocate any more. */
 
   mapped_name_t *insert = tor_malloc_zero(
-                       offsetof(mapped_name_t, name) + namelen + 1);
+            offsetof(mapped_name_t, name) + namelen + 1
+            /* Unfortunate hack to let valgrind handle SSE optimizations
+             * in strlen in -O2. See bug 30996. */
+            + 4);
   memcpy(insert->name, name, namelen+1);
   insert->intval = new_id;

to my list of patches that make moria1 different from master.

(I started with a slightly more complex hack, with a bufsize variable and a -= and a %4, but a hack is a hack so I'll not quibble. :)

I'll let you decide if you want to close this as a wontfix and I'll just maintain my workaround forever in my growing list of differences, or if you want to take my hack into Tor itself.

comment:5 Changed 3 weeks ago by arma

(I am using a much newer valgrind than in that bugzilla report, but, that doesn't seem to help me here either. :)

comment:6 Changed 29 hours ago by nickm

Let's look into your setup some more: I don't see this on Fedora 30 with valgrind 3.15 and glibc 2.29. Can we figure out what combination of versions is needed to cause this behavior?

comment:7 Changed 29 hours ago by nickm

Status: acceptedneeds_information

comment:8 Changed 28 hours ago by kushaldas

I can not see the similar errors while running under valgrind.

Jul 19 21:17:32.019 [notice] Tor 0.4.2.0-alpha-dev (git-5303dbe6249ed4b1) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.1.0k, Zlib 1.2.8, Liblzma 5.2.2, and Libzstd N/A.

valgrind 3.12.0.SVN, Debian GLIBC 2.24-11+deb9u4 on Debian Stretch here.

Last edited 28 hours ago by kushaldas (previous) (diff)

comment:9 Changed 27 hours ago by arma

$ valgrind --version
valgrind-3.8.1
$ rpm -qa|grep glibc
glibc-debuginfo-common-2.12-1.209.el6_9.2.x86_64
glibc-debuginfo-2.12-1.209.el6_9.2.x86_64
glibc-devel-2.12-1.212.el6_10.3.x86_64
glibc-common-2.12-1.212.el6_10.3.x86_64
glibc-2.12-1.212.el6_10.3.x86_64
glibc-headers-2.12-1.212.el6_10.3.x86_64

comment:10 Changed 24 hours ago by nickm

Okay. My working theory is that this is a problem solved with later versions of valgrind and/or glibc. I'm leaning towards "wontfix" on this, since the valgrind is 7 years old and the glibc is 9 years old and it isn't actually a bug in the code.

Note: See TracTickets for help on using tickets.