From: Ilpo Järvinen (ilpo.jarvinen@helsinki.fi)
Date: 01/04/06
Date: Wed, 4 Jan 2006 17:51:19 +0200 (EET) From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi> Subject: Re: tcptrace-bugs Static variable returns of HostName() are not consider by callers Message-ID: <Pine.LNX.4.58.0601041708590.21085@kivilampi-30.cs.helsinki.fi>
On Tue, 3 Jan 2006, Manikantan Ramadas wrote:
> You are certainly right in that many Network Socket Library implementations
> of gethostbyaddr tend to use static storage internally. They perhaps cycle
> thru' a bunch of static memory locations.
>
> But I think tcptrace is safe on this one. I looked at the relevant code
> sections you point out. HostName() function first looks up its own internal
> cache before making the expensive gethostbyaddr call; but since the cainsert
> routine (used to insert an entry into the cache upon a cache-miss) makes a
> copy of the hostname found from gethostbyaddr, we are protected from cache
> poisoning.
This is a caller issue. I'm sorry I didn't make it clear enough in the
last posting... As long as the caller does not depend a value returned
earlier by HostName after a second HostName call, we should be safe. But
when it is called twice, as in some debug printf()s, the later call can
overwrite the previous content of the name_buf, or the static return
buffer of the gethostbyaddr().
> Now HostName function either copies an entry found from the cache into its
> own internal static variable name_buf and returns it, or returns the char* as
> returned from gethostbyaddr. But as far as I can see, tcptrace code seems
> cognizant of this fact; Note that all the per-connection data-structure's
> hostname entries such as
> ptp->a_hostname, ptp->b_hostname (trace.c) make an strdup call on what the
> HostName returns.
That's correct thing to do (or not to call HostName again until the
last return value is no longer needed).
> Every where else where HostName() gets called, it is merely
> for debug-help printf messages;
I agree, the hazard is there but it is ignored only in few debug printf()
(AFAIK). It really *did* manifest as wrong behavior that made debugging of
another bug in 6.6.1 (when dst_port==src_port, seems to be fixed now in
6.6.7) a lot harder until I fixed the printf()s... It's not nice though
suggesting that debugging printf()s are allowed to mislead, which was
the case in this instance...
> As long the HostName char* pointers don't get
> corrupted within the printf's, we should be alright.
This is incorrect thinking! The static char name_buf[100] in HostName()
is used twice within a single printf(). The printf does "duplicate" the
string but too late, as it's not called until all arguments have been
calculated. Let me describe how things end up wrong:
1. HostName looks up the first address and copies it to name_buf in
calookup
2. HostName returns pointer to name_buf
3. The second address is then looked up and copied to the name_buf, which
overwrites the previous address from the static buffer.
4. We have two pointers in printf(), which point to the same static
name_buf of HostName(), but only the later address is stored there...
Or, with gethostbyaddr():
1. HostName does not find entry from cache
2. It does phe = gethostbyaddr()
3. phe->h_name is returned (yes, its the static one), even though the
cache is not trashed as copying happens in cainsert()! Returning pointer
to the cached string would fix this one...
4. The second call, again, falls back to gethostbyaddr() call, which
possibly corrupts the data that was returned earlier (depending on
the pooling of the static memory locations of gethostbyaddr())
> We know of folks using tcptrace to process GB dumpfiles; therefore making
> such complex memory optimization with statics seem worth the code-complexity
> introduced.
Yes, I agree with the optimization, but not with the caller behavior of
the debug printf()s.
Splitting the relevant printf()s should be enough, like:
printf("%s %s", HostName(addr1), HostName(addr2));
-> printf("%s ", Hostname(addr1)); printf("%s", HostName(addr2);
-- i.
This archive was generated by hypermail 2.1.7 : 01/04/06 EST