New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 7 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Valgrind reports overlapping memory regions in calls to memcpy

Reported by chapp...@gmail.com, Dec 15 2014

Issue description

Overview

I suspect that the issue is with the memcpy utilized by buffer.h. It should likely be changed to use a memmove.

What steps will reproduce the problem?

1. Run a simple c++ native app where a webrtc data channel is created/initiated under valgrind.
2. Allow the application to shutdown naturally or through a registered SIGINT handler.

What is the expected result?

I expect the application to shutdown without memory errors.

What do you see instead?

Instead I see the following valgrind output:

==3561== Source and destination overlap in memcpy(0x11065620, 0x11065620, 48)
==3561==    at 0x4C2F71C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3561==    by 0x138B3CB: rtc::MessageQueue::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (messagequeue.cc:373)
==3561==    by 0x13B41EE: rtc::Thread::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (thread.cc:497)
==3561==    by 0x143F7BA: cricket::BaseChannel::FlushRtcpMessages() (channel.cc:1245)
==3561==    by 0x143F300: cricket::BaseChannel::~BaseChannel() (channel.cc:185)
==3561==    by 0x144F144: cricket::DataChannel::~DataChannel() (channel.cc:2117)
==3561==    by 0x144F378: cricket::DataChannel::~DataChannel() (channel.cc:2111)
==3561==    by 0x147B2BD: cricket::ChannelManager::DestroyDataChannel_w(cricket::DataChannel*) (channelmanager.cc:463)
==3561==    by 0x14823F8: rtc::MethodFunctor1<cricket::ChannelManager, void (cricket::ChannelManager::*)(cricket::DataChannel*), void, cricket::DataChannel*>::operator()() const (bind.h:122)
==3561==    by 0x1482372: rtc::FunctorMessageHandler<void, rtc::MethodFunctor1<cricket::ChannelManager, void (cricket::ChannelManager::*)(cricket::DataChannel*), void, cricket::DataChannel*> >::OnMessage(rtc::Message*) (messagehandler.h:57)
==3561==    by 0x13B3F76: rtc::Thread::ReceiveSends() (thread.cc:464)
==3561==    by 0x138A691: rtc::MessageQueue::Get(rtc::Message*, int, bool) (messagequeue.cc:187)
==3561== 
==3561== Source and destination overlap in memcpy(0x11065620, 0x11065620, 48)
==3561==    at 0x4C2F71C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3561==    by 0x138B3CB: rtc::MessageQueue::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (messagequeue.cc:373)
==3561==    by 0x13B41EE: rtc::Thread::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (thread.cc:497)
==3561==    by 0x143F33A: cricket::BaseChannel::~BaseChannel() (channel.cc:186)
==3561==    by 0x144F144: cricket::DataChannel::~DataChannel() (channel.cc:2117)
==3561==    by 0x144F378: cricket::DataChannel::~DataChannel() (channel.cc:2111)
==3561==    by 0x147B2BD: cricket::ChannelManager::DestroyDataChannel_w(cricket::DataChannel*) (channelmanager.cc:463)
==3561==    by 0x14823F8: rtc::MethodFunctor1<cricket::ChannelManager, void (cricket::ChannelManager::*)(cricket::DataChannel*), void, cricket::DataChannel*>::operator()() const (bind.h:122)
==3561==    by 0x1482372: rtc::FunctorMessageHandler<void, rtc::MethodFunctor1<cricket::ChannelManager, void (cricket::ChannelManager::*)(cricket::DataChannel*), void, cricket::DataChannel*> >::OnMessage(rtc::Message*) (messagehandler.h:57)
==3561==    by 0x13B3F76: rtc::Thread::ReceiveSends() (thread.cc:464)
==3561==    by 0x138A691: rtc::MessageQueue::Get(rtc::Message*, int, bool) (messagequeue.cc:187)
==3561==    by 0x13B3A6B: rtc::Thread::ProcessMessages(int) (thread.cc:516)
==3561== 
==3561== Source and destination overlap in memcpy(0x11065620, 0x11065620, 48)
==3561==    at 0x4C2F71C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3561==    by 0x138B3CB: rtc::MessageQueue::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (messagequeue.cc:373)
==3561==    by 0x13B41EE: rtc::Thread::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (thread.cc:497)
==3561==    by 0x1394685: rtc::BasicNetworkManager::StopUpdating() (network.cc:549)
==3561==    by 0x1429CBD: cricket::BasicPortAllocatorSession::~BasicPortAllocatorSession() (basicportallocator.cc:256)
==3561==    by 0x142A328: cricket::BasicPortAllocatorSession::~BasicPortAllocatorSession() (basicportallocator.cc:255)
==3561==    by 0x14C8251: rtc::DefaultDeleter<cricket::PortAllocatorSession>::operator()(cricket::PortAllocatorSession*) const (scoped_ptr.h:138)
==3561==    by 0x14C8213: rtc::internal::scoped_ptr_impl<cricket::PortAllocatorSession, rtc::DefaultDeleter<cricket::PortAllocatorSession> >::~scoped_ptr_impl() (scoped_ptr.h:214)
==3561==    by 0x14C2DA4: rtc::scoped_ptr<cricket::PortAllocatorSession, rtc::DefaultDeleter<cricket::PortAllocatorSession> >::~scoped_ptr() (scoped_ptr.h:441)
==3561==    by 0x14C2B47: cricket::PortAllocatorSessionMuxer::~PortAllocatorSessionMuxer() (portallocatorsessionproxy.cc:59)
==3561==    by 0x14C2DE8: cricket::PortAllocatorSessionMuxer::~PortAllocatorSessionMuxer() (portallocatorsessionproxy.cc:54)
==3561==    by 0x14C3133: cricket::PortAllocatorSessionMuxer::OnSessionProxyDestroyed(cricket::PortAllocatorSession*) (portallocatorsessionproxy.cc:113)
==3561== 
==3561== Source and destination overlap in memcpy(0x11065620, 0x11065620, 48)
==3561==    at 0x4C2F71C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3561==    by 0x138B3CB: rtc::MessageQueue::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (messagequeue.cc:373)
==3561==    by 0x13B41EE: rtc::Thread::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (thread.cc:497)
==3561==    by 0x1429D09: cricket::BasicPortAllocatorSession::~BasicPortAllocatorSession() (basicportallocator.cc:258)
==3561==    by 0x142A328: cricket::BasicPortAllocatorSession::~BasicPortAllocatorSession() (basicportallocator.cc:255)
==3561==    by 0x14C8251: rtc::DefaultDeleter<cricket::PortAllocatorSession>::operator()(cricket::PortAllocatorSession*) const (scoped_ptr.h:138)
==3561==    by 0x14C8213: rtc::internal::scoped_ptr_impl<cricket::PortAllocatorSession, rtc::DefaultDeleter<cricket::PortAllocatorSession> >::~scoped_ptr_impl() (scoped_ptr.h:214)
==3561==    by 0x14C2DA4: rtc::scoped_ptr<cricket::PortAllocatorSession, rtc::DefaultDeleter<cricket::PortAllocatorSession> >::~scoped_ptr() (scoped_ptr.h:441)
==3561==    by 0x14C2B47: cricket::PortAllocatorSessionMuxer::~PortAllocatorSessionMuxer() (portallocatorsessionproxy.cc:59)
==3561==    by 0x14C2DE8: cricket::PortAllocatorSessionMuxer::~PortAllocatorSessionMuxer() (portallocatorsessionproxy.cc:54)
==3561==    by 0x14C3133: cricket::PortAllocatorSessionMuxer::OnSessionProxyDestroyed(cricket::PortAllocatorSession*) (portallocatorsessionproxy.cc:113)
==3561==    by 0x14C7054: sigslot::_connection1<cricket::PortAllocatorSessionMuxer, cricket::PortAllocatorSession*, sigslot::single_threaded>::emit(cricket::PortAllocatorSession*) (sigslot.h:1852)
==3561== 
==3561== Source and destination overlap in memcpy(0x11065620, 0x11065620, 48)
==3561==    at 0x4C2F71C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3561==    by 0x138B3CB: rtc::MessageQueue::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (messagequeue.cc:373)
==3561==    by 0x13B41EE: rtc::Thread::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (thread.cc:497)
==3561==    by 0x142E342: cricket::AllocationSequence::~AllocationSequence() (basicportallocator.cc:739)
==3561==    by 0x142E518: cricket::AllocationSequence::~AllocationSequence() (basicportallocator.cc:738)
==3561==    by 0x1429EEA: cricket::BasicPortAllocatorSession::~BasicPortAllocatorSession() (basicportallocator.cc:274)
==3561==    by 0x142A328: cricket::BasicPortAllocatorSession::~BasicPortAllocatorSession() (basicportallocator.cc:255)
==3561==    by 0x14C8251: rtc::DefaultDeleter<cricket::PortAllocatorSession>::operator()(cricket::PortAllocatorSession*) const (scoped_ptr.h:138)
==3561==    by 0x14C8213: rtc::internal::scoped_ptr_impl<cricket::PortAllocatorSession, rtc::DefaultDeleter<cricket::PortAllocatorSession> >::~scoped_ptr_impl() (scoped_ptr.h:214)
==3561==    by 0x14C2DA4: rtc::scoped_ptr<cricket::PortAllocatorSession, rtc::DefaultDeleter<cricket::PortAllocatorSession> >::~scoped_ptr() (scoped_ptr.h:441)
==3561==    by 0x14C2B47: cricket::PortAllocatorSessionMuxer::~PortAllocatorSessionMuxer() (portallocatorsessionproxy.cc:59)
==3561==    by 0x14C2DE8: cricket::PortAllocatorSessionMuxer::~PortAllocatorSessionMuxer() (portallocatorsessionproxy.cc:54)
==3561== 
==3561== Thread 17:
==3561== Source and destination overlap in memcpy(0x11065620, 0x11065620, 48)
==3561==    at 0x4C2F71C: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==3561==    by 0x138B3CB: rtc::MessageQueue::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (messagequeue.cc:373)
==3561==    by 0x13B41EE: rtc::Thread::Clear(rtc::MessageHandler*, unsigned int, std::list<rtc::Message, std::allocator<rtc::Message> >*) (thread.cc:497)
==3561==    by 0x14188DD: cricket::Transport::DestroyAllChannels() (transport.cc:369)
==3561==    by 0x13EDA0A: cricket::DtlsTransport<cricket::P2PTransport>::~DtlsTransport() (dtlstransport.h:57)
==3561==    by 0x13EDA48: cricket::DtlsTransport<cricket::P2PTransport>::~DtlsTransport() (dtlstransport.h:56)
==3561==    by 0x13F0571: rtc::DefaultDeleter<cricket::Transport>::operator()(cricket::Transport*) const (scoped_ptr.h:138)
==3561==    by 0x13F0533: rtc::internal::scoped_ptr_impl<cricket::Transport, rtc::DefaultDeleter<cricket::Transport> >::~scoped_ptr_impl() (scoped_ptr.h:214)
==3561==    by 0x13F04F4: rtc::scoped_ptr<cricket::Transport, rtc::DefaultDeleter<cricket::Transport> >::~scoped_ptr() (scoped_ptr.h:441)
==3561==    by 0x13F049B: rtc::RefCountedObject<rtc::scoped_ptr<cricket::Transport, rtc::DefaultDeleter<cricket::Transport> > >::~RefCountedObject() (refcount.h:71)
==3561==    by 0x13F04C8: rtc::RefCountedObject<rtc::scoped_ptr<cricket::Transport, rtc::DefaultDeleter<cricket::Transport> > >::~RefCountedObject() (refcount.h:70)
==3561==    by 0x13F0471: rtc::RefCountedObject<rtc::scoped_ptr<cricket::Transport, rtc::DefaultDeleter<cricket::Transport> > >::Release() (refcount.h:64)
==3561== 


What version of the product are you using? On what operating system?

Running on ubuntu 14.04 X86_64

Please provide any additional information below.



 
Project Member

Comment 1 by juberti@webrtc.org, Dec 15 2014

Labels: Area-Network
Project Member

Comment 2 by pthatcher@webrtc.org, Dec 15 2014

Labels: EngTriaged Mstone-43
Project Member

Comment 3 by juberti@webrtc.org, Dec 16 2014

Cc: pthatcher@webrtc.org decurtis@webrtc.org
Status: Available
I looked at the code here - the issue is pretty obvious, but probably harmless.

See messagequeue.cc:
https://code.google.com/p/webrtc/source/browse/trunk/webrtc/base/messagequeue.cc?line=360#360

 PriorityQueue::container_type::iterator new_end = dmsgq_.container().begin();
  for (PriorityQueue::container_type::iterator it = new_end;
       it != dmsgq_.container().end(); ++it) {
    if (it->msg_.Match(phandler, id)) {
      if (removed) {
        removed->push_back(it->msg_);
      } else {
        delete it->msg_.pdata;
      }
    } else {
      *new_end++ = *it;   <--------------- causes memcpy
    }
  }

If |it| and |new_end| are equal, this causes a self-assignment. The simple fix here is just to either:

 change the final else to
{
  if (new_end != it) 
     *new_end = *it;
  new_end++;
}

or add an assignment op in the Message class that checks and no-ops on self-assignment.
}

else if (new_end != it)

Project Member

Comment 4 by decurtis@webrtc.org, Jan 14 2015

Owner: decurtis@webrtc.org
Status: Started
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 21 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/2bffc3cb72e2250cbf6ed7e5f4b399395ca046cb

commit 2bffc3cb72e2250cbf6ed7e5f4b399395ca046cb
Author: decurtis@webrtc.org <decurtis@webrtc.org>
Date: Sat Feb 21 01:45:04 2015

When clearing the priority message queue, don't copy an item to itself.

This avoids a memcpy to overlapping---in this case the same---memory locations.

BUG=4100
R=juberti@webrtc.org, pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/33019004

Cr-Commit-Position: refs/heads/master@{#8449}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8449 4adac7df-926f-26a2-2b94-8c16560cd09d

[modify] http://crrev.com/2bffc3cb72e2250cbf6ed7e5f4b399395ca046cb/webrtc/base/messagequeue.cc
[modify] http://crrev.com/2bffc3cb72e2250cbf6ed7e5f4b399395ca046cb/webrtc/base/messagequeue_unittest.cc

Project Member

Comment 7 by decurtis@webrtc.org, Mar 31 2015

Status: Fixed
Project Member

Comment 8 by decurtis@webrtc.org, Apr 21 2015

Status: Assigned
Project Member

Comment 9 by decurtis@webrtc.org, Apr 21 2015

Labels: -Mstone-43 Mstone-44
Project Member

Comment 10 by tnakamura@webrtc.org, Jan 29 2016

Labels: -Mstone-44
I don't see any recent CLs linked to this bug, so I don't think it's been fixed. I'm therefore leaving this in an open state, but I am removing the milestone label since this bug hasn't been updated in quite some time.
Project Member

Comment 11 by pthatcher@webrtc.org, Nov 8 2016

Labels: Pri-3
Project Member

Comment 12 by deadbeef@chromium.org, Apr 3

Owner: ----
Status: Available (was: Assigned)
Clearing owner and setting status to Available, since there haven't been any updates for > 1 year. Will be assigned again once priority is high enough.

Sign in to add a comment