Re: Valgrind results
On Tue, 2008-03-25 at 10:23 +1300, Amos Jeffries wrote: I found that in 3.x as well. It turns out those bits of valgrind code only occur if both --enable-leakfinder and --with-valgrind-debug are enabled. My test script only had --with-valgrind which wasn't doing even the job it needed to :-( Odd.. FWIW: Is there any reason why we need separate leakfinder and valgrind modes? valgrind should implicitly enable leakfinder I think. The leakfinder code is independent from valgrind however. And additionally the code fragment discussed should not even depend on leakfinder, only valgrind.. Regards Henrik
Re: Valgrind results
On Sun, 2008-03-23 at 20:37 +0900, Adrian Chadd wrote: On Mon, Mar 24, 2008, Amos Jeffries wrote: Adrian Chadd wrote: You can ignore that. If you care that much about the epoll error then just bzero() the data being passed into epoll_ctl() before its called. I care about every potential cause of trouble in squid. Particularly avoidable ones that pop up all the time. 'Zero errors' policy and all that. Thats cool. There's just bigger fish to fry. :) It may be that this is fixable by setting the first byte of the buffer to '\0'. I'm just busy working on the other more serious ones myself tonight. Nah, it'll actually check the whole region. Is it valgrind being wrong, or does epoll actually read from that unitinitialised region? If the former, thats what excludes are for; if the latter then it is a bug :) -Rob -- GPG key available at: http://www.robertcollins.net/keys.txt. signature.asc Description: This is a digitally signed message part
Re: Valgrind results
Henrik Nordstrom wrote: On Tue, 2008-03-25 at 10:23 +1300, Amos Jeffries wrote: I found that in 3.x as well. It turns out those bits of valgrind code only occur if both --enable-leakfinder and --with-valgrind-debug are enabled. My test script only had --with-valgrind which wasn't doing even the job it needed to :-( Odd.. Twas until I realised it changed in -3 to have a -debugs or somesuch on the end. My scripts were still using the -2 option :-( FWIW: Is there any reason why we need separate leakfinder and valgrind modes? valgrind should implicitly enable leakfinder I think. The leakfinder code is independent from valgrind however. And additionally the code fragment discussed should not even depend on leakfinder, only valgrind.. Okay. I'll fix that then. Amos -- Please use Squid 2.6STABLE17+ or 3.0STABLE1+ There are serious security advisories out on all earlier releases.
Re: Valgrind results
On Tue, 2008-03-25 at 12:23 +1100, Robert Collins wrote: Is it valgrind being wrong, or does epoll actually read from that unitinitialised region? it's valgrind not knowing (or caring about) the full details of when epoll uses what of the supplied data. But generally speaking it's a good warning as supplying uninitialized input data to the kernel even if that data is currently unused for the specific call details is perhaps not a good idea. Regards Henrik
Re: Valgrind results
On Mon, 2008-03-24 at 00:16 +1300, Amos Jeffries wrote:
I care about every potential cause of trouble in squid. Particularly
avoidable ones that pop up all the time. 'Zero errors' policy and all that.
It may be that this is fixable by setting the first byte of the buffer
to '\0'. I'm just busy working on the other more serious ones myself
tonight.
You need to set the whole buffer to 0.
In Squid-2 we have the following fragment to deal with this warning:
if (RUNNING_ON_VALGRIND) {
/* Keep valgrind happy.. complains about uninitialized bytes otherwise
*/
memset(ev, 0, sizeof(ev));
}
Alternatively one can spend a little time to teach valgrind about how
epoll_ctl reads it's parameters.. The warning comes from the data member
being an union of different sizes depending on the epoll_ctl call used..
Regards
Henrik
Re: Valgrind results
Henrik Nordstrom wrote:
On Mon, 2008-03-24 at 00:16 +1300, Amos Jeffries wrote:
I care about every potential cause of trouble in squid. Particularly
avoidable ones that pop up all the time. 'Zero errors' policy and all that.
It may be that this is fixable by setting the first byte of the buffer
to '\0'. I'm just busy working on the other more serious ones myself
tonight.
You need to set the whole buffer to 0.
In Squid-2 we have the following fragment to deal with this warning:
if (RUNNING_ON_VALGRIND) {
/* Keep valgrind happy.. complains about uninitialized bytes otherwise
*/
memset(ev, 0, sizeof(ev));
}
I found that in 3.x as well. It turns out those bits of valgrind code
only occur if both --enable-leakfinder and --with-valgrind-debug are
enabled.
My test script only had --with-valgrind which wasn't doing even the
job it needed to :-(
FWIW: Is there any reason why we need separate leakfinder and valgrind
modes?
Alternatively one can spend a little time to teach valgrind about how
epoll_ctl reads it's parameters.. The warning comes from the data member
being an union of different sizes depending on the epoll_ctl call used..
Regards
Henrik
Amos
--
Please use Squid 2.6STABLE17+ or 3.0STABLE1+
There are serious security advisories out on all earlier releases.
Re: Valgrind results
You can ignore that. If you care that much about the epoll error then just bzero() the data being passed into epoll_ctl() before its called. adrian On Mon, Mar 24, 2008, Amos Jeffries wrote: Just doing a quick check with valgrind I'm seeing a lot of errors in the event system. Syscall param epoll_ctl(event) points to uninitialised byte(s) with a wide range of events like: ==25761==at 0x4559A59: epoll_ctl (in /lib/libc-2.7.so) ==25761==by 0x8171C2F: comm_read(int, char*, int, RefCountAsyncCall) (comm.cc:346) ==25761==by 0x814B50C: StoreEntry::delayAwareRead(int, char*, int, RefCountAsyncCall) (store.cc:261) ==25761==by 0x80FB573: HttpStateData::maybeReadVirginBody() (http.cc:1290) ==25761==by 0x80F9C36: HttpStateData::sendRequest() (http.cc:1782) ==25761==by 0x80FA594: httpStart (http.cc:1845) ==25761==by 0x817AF86: CommConnectCbPtrFun::dial() (CommCalls.cc:127) ==25761==by 0x8069A27: AsyncCall::make() (AsyncCall.cc:34) ==25761==by 0x8068EAC: AsyncCallQueue::fireNext() (AsyncCallQueue.cc:53) ==25761==by 0x8068FAD: AsyncCallQueue::fire() (AsyncCallQueue.cc:39) ==25761==by 0x80CF392: EventLoop::dispatchCalls() (EventLoop.cc:156) ==25761==by 0x80CF58C: EventLoop::runOnce() (EventLoop.cc:133) ==25761== Address 0xbef187d8 is on thread 1's stack All sharing the root: comm_read(int, char*, int, RefCountAsyncCall) (comm.cc:346) Anyone want to attack this one? Amos -- Please use Squid 2.6STABLE17+ or 3.0STABLE1+ There are serious security advisories out on all earlier releases. -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: Valgrind results
Adrian Chadd wrote: You can ignore that. If you care that much about the epoll error then just bzero() the data being passed into epoll_ctl() before its called. I care about every potential cause of trouble in squid. Particularly avoidable ones that pop up all the time. 'Zero errors' policy and all that. It may be that this is fixable by setting the first byte of the buffer to '\0'. I'm just busy working on the other more serious ones myself tonight. Amos -- Please use Squid 2.6STABLE17+ or 3.0STABLE1+ There are serious security advisories out on all earlier releases.
Re: Valgrind results
On Mon, Mar 24, 2008, Amos Jeffries wrote: Adrian Chadd wrote: You can ignore that. If you care that much about the epoll error then just bzero() the data being passed into epoll_ctl() before its called. I care about every potential cause of trouble in squid. Particularly avoidable ones that pop up all the time. 'Zero errors' policy and all that. Thats cool. There's just bigger fish to fry. :) It may be that this is fixable by setting the first byte of the buffer to '\0'. I'm just busy working on the other more serious ones myself tonight. Nah, it'll actually check the whole region. -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
Re: Valgrind results
Adrian Chadd wrote: On Mon, Mar 24, 2008, Amos Jeffries wrote: Adrian Chadd wrote: You can ignore that. If you care that much about the epoll error then just bzero() the data being passed into epoll_ctl() before its called. I care about every potential cause of trouble in squid. Particularly avoidable ones that pop up all the time. 'Zero errors' policy and all that. Thats cool. There's just bigger fish to fry. :) Like why '--with-valgrind' doesn't enable the initialisation code somebody already added to squid for those minor ones Trying leakfinder + valgrind now. It may be that this is fixable by setting the first byte of the buffer to '\0'. I'm just busy working on the other more serious ones myself tonight. Nah, it'll actually check the whole region. Amos -- Please use Squid 2.6STABLE17+ or 3.0STABLE1+ There are serious security advisories out on all earlier releases.
Re: Valgrind results
On Mon, Mar 24, 2008, Amos Jeffries wrote: Thats cool. There's just bigger fish to fry. :) Like why '--with-valgrind' doesn't enable the initialisation code somebody already added to squid for those minor ones Trying leakfinder + valgrind now. Heh, valgrind makes it mostly easy to ignore those; there should only be a handful of places where epoll and the like get called and thus only a handful of false positives to sift through. It'll be interesting to see whether other leaks show up during heavy use. Adrian -- - Xenion - http://www.xenion.com.au/ - VPS Hosting - Commercial Squid Support - - $25/pm entry-level VPSes w/ capped bandwidth charges available in WA -
