How about something like this:
Index: pbuf.c
===================================================================
RCS file: /export/home/ntop/ntop/pbuf.c,v
retrieving revision 2.137
diff -U3 -r2.137 pbuf.c
--- pbuf.c 28 Aug 2003 16:17:55 -0000 2.137
+++ pbuf.c 11 Sep 2003 16:23:16 -0000
@@ -955,28 +955,30 @@
int WIN=0, MSS=-1, WS=-1, S=0, N=0, D=0, T=0;
int ttl;
char WSS[3], _MSS[5];
- struct tcphdr *tcp = (struct tcphdr*)(bp+hlen);
- u_char *tcp_opt = (u_char *)(tcp + 1);
- u_char *tcp_data = (u_char *)((int)tcp + tcp->th_off * 4);
- if (tcp->th_flags & TH_SYN) /* only SYN or SYN-2ACK packets */
+ if (tp.th_flags & TH_SYN) /* only SYN or SYN-2ACK packets */
{
if (tcpUdpLen > 0) {
if(ntohs(ip.ip_off) & IP_DF) D = 1; /* don't fragment bit is set */
- WIN = ntohs(tcp->th_win); /* TCP window size */
+ WIN = ntohs(tp.th_win); /* TCP window size */
- if (tcp_data != tcp_opt) /* there are some tcp_option to be parsed */
+ if ((tp.th_off * 4) >= sizeof(struct tcphdr)) /* there are some
tcp_option to be parsed */
{
- u_char *opt_ptr = tcp_opt;
+ u_char *opt_ptr, *opt;
+ int opt_size = tp.th_off * 4;
- while(opt_ptr < tcp_data)
+ opt = malloc((size_t)opt_size);
+ memcpy(opt, bp+hlen+sizeof(struct tcphdr), opt_size);
+ opt_ptr = opt;
+
+ while(opt_ptr < (opt + opt_size))
{
switch(*opt_ptr)
{
case TCPOPT_EOL: /* end option: exit */
- opt_ptr = tcp_data;
+ opt_ptr = opt + opt_size;
break;
case TCPOPT_NOP:
N = 1;
@@ -1006,6 +1008,7 @@
opt_ptr += (*opt_ptr - 1);
break;
}
+ free(opt);
}
}
@@ -1018,7 +1021,7 @@
snprintf(fingerprint, sizeof(fingerprint),
"%04X:%s:%02X:%s:%d:%d:%d:%d:%c:%02X",
WIN, _MSS, ttl = TTL_PREDICTOR(ip.ip_ttl), WSS , S, N, D, T,
- (tcp->th_flags & TH_ACK) ? 'A' : 'S', tcpUdpLen);
+ (tp.th_flags & TH_ACK) ? 'A' : 'S', tcpUdpLen);
#if 0
traceEvent(CONST_TRACE_INFO, "[%s][%s]", srcHost->hostNumIpAddress,
fingerprint);
Works better too:)
-Chris
>
> Yea I think I missed the fact that tp isn't getting enough of the packet
> to handle options. I don't think we can copy more into tp because we
> don't know and may not have options at that point. I think we'll need to
> memcpy a new tcpheader+options inside the SYN loop. Or maybe just
> memcpy the options to a new pointer? I'll try and test something
> today.
> -Chris
> >
> > struct tcphdr defines the tcp/ip standard header. Following that is up to
> > 40 bytes of 'options'. Since the def of tp is 'struct tcphdr tp;', any
> > reference using it into the opts will be seen as an uninitialized reference.
> >
> > I've been routinely applying Chris' patch for the un-aligned moves/sparc
> > problem, and that may be what Valgrind is catching. He changes the
> > definitions to:
> >
> > - u_char *opt_ptr = tcp_opt;
> > + u_char *opt_ptr = (u_char *)((struct tcphdr*)&tp + 1);
> > + u_char *tcp_data =
> > + (u_char *)((struct tcphdr*)&tp + tp.th_off * 4);
> >
> > I just don't know - it's not clear how deep valgrind goes with it's
> > analysis - it could be that it sees the 24 byte header being overflowed or
> > it could be picking up something else about the code.
> >
> > Since you're going to be in there anyway, looking at Chris' patch, I wanted
> > you to look at it and make sure the code is ok.
> >
> >
> > Once you're sure that the code is right, I'll try and code a suppress for
> > Valgrind for this.
> >
> > -----Burton
> >
> >
> > -----Original Message-----
> > From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Behalf
> > Of Luca Deri
> > Sent: Thursday, September 11, 2003 3:53 AM
> > To: [EMAIL PROTECTED]
> > Subject: Re: [Ntop-dev] VALGRIND bug report
> >
> >
> > Burton,
> > I can't follow you here. opt_ptr is the problem right? If so:
> >
> > struct tcphdr *tcp = (struct tcphdr*)(bp+hlen);
> > u_char *tcp_opt = (u_char *)(tcp + 1);
> > u_char *opt_ptr = tcp_opt;
> >
> > In the above statements I can't see tp being used at all, so I don't
> > understand the memcpy problem.
> >
> > Please explain.
> >
> > Rgds, Luca
> >
> > Burton M. Strauss III wrote:
> >
> > >==922== Thread 2:
> > >==922== Conditional jump or move depends on uninitialised value(s)
> > >==922== at 0x402AA52A: processIpPkt (pbuf.c:975)
> > >==922== by 0x402AD189: processPacket (pbuf.c:2513)
> > >==922== by 0x402AB37C: dequeuePacket (pbuf.c:1735)
> > >==922== by 0x4045B5C2: thread_wrapper (vg_libpthread.c:667)
> > >==922==
> > >==922== Thread 2:
> > >==922== Use of uninitialised value of size 4
> > >==922== at 0x402AA536: processIpPkt (pbuf.c:975)
> > >==922== by 0x402AD189: processPacket (pbuf.c:2513)
> > >==922== by 0x402AB37C: dequeuePacket (pbuf.c:1735)
> > >==922== by 0x4045B5C2: thread_wrapper (vg_libpthread.c:667)
> > >
> > >
> > > if(srcHost->fingerprint == NULL) {
> > > char fingerprint[64];
> > > int WIN=0, MSS=-1, WS=-1, S=0, N=0, D=0, T=0;
> > > int ttl;
> > > char WSS[3], _MSS[5];
> > >
> > > if (tp.th_flags & TH_SYN) /* only SYN or SYN-2ACK packets */
> > > {
> > >...
> > > while(opt_ptr < tcp_data)
> > > {
> > >
> > >
> > >>>> switch(*opt_ptr)
> > >>>>
> > >>>>
> > > {
> > >...
> > > }
> > > }
> > > }
> > >...
> > > }
> > >
> > >
> > >Luca, I think valgrind is right, because the set of tp is this:
> > >
> > > memcpy(&tp, bp+hlen, sizeof(struct tcphdr));
> > >
> > >(pbuf.c @ 921), which doesn't copy any of the option fields. We should
> > >probably add 40 bytes (that's the maximum length of the options field) to
> > >that memcpy...
> > >
> > >-----Burton
> > >
> > >_______________________________________________
> > >Ntop-dev mailing list
> > >[EMAIL PROTECTED]
> > >http://listgateway.unipi.it/mailman/listinfo/ntop-dev
> > >
> > >
> >
> >
> > --
> > Luca Deri <[EMAIL PROTECTED]> http://luca.ntop.org/
> > Hacker: someone who loves to program and enjoys being
> > clever about it - Richard Stallman
> >
> >
> > _______________________________________________
> > Ntop-dev mailing list
> > [EMAIL PROTECTED]
> > http://listgateway.unipi.it/mailman/listinfo/ntop-dev
> >
> > _______________________________________________
> > Ntop-dev mailing list
> > [EMAIL PROTECTED]
> > http://listgateway.unipi.it/mailman/listinfo/ntop-dev
> >
>
>
> --
> [EMAIL PROTECTED] Chris Turbeville NTT/VERIO
> Send mail with subject "send PGP Key" for PGP 6.5.2 Public key
> _______________________________________________
> Ntop-dev mailing list
> [EMAIL PROTECTED]
> http://listgateway.unipi.it/mailman/listinfo/ntop-dev
>
--
[EMAIL PROTECTED] Chris Turbeville NTT/VERIO
Send mail with subject "send PGP Key" for PGP 6.5.2 Public key
_______________________________________________
Ntop-dev mailing list
[EMAIL PROTECTED]
http://listgateway.unipi.it/mailman/listinfo/ntop-dev
