diff --git a/CHANGELOG b/CHANGELOG index 97de2ff5..07e18937 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -66,7 +66,12 @@ HISTORY * api_lib.c: Use memcpy in netbuf_copy_partial. ++ Bug fixes: - + 2007-03-21 Kieran Mansley + * Fix all uses of pbuf_header to check the return value. In some + cases just assert if it fails as I'm not sure how to fix them, but + this is no worse than before when they would carry on regardless + of the failure. + 2007-03-21 Kieran Mansley * sockets.c, igmp.c, igmp.h, memp.h: Fix C++ style comments and comment out missing header include in icmp.c diff --git a/src/api/tcpip.c b/src/api/tcpip.c index c3457759..5d742909 100644 --- a/src/api/tcpip.c +++ b/src/api/tcpip.c @@ -138,9 +138,14 @@ ethernet_input(struct pbuf *p, struct netif *netif) etharp_ip_input( netif, p); #endif /* skip Ethernet header */ - pbuf_header(p, (s16_t)(-sizeof(struct eth_hdr))); - /* pass to IP layer */ - ip_input(p, netif); + if(pbuf_header(p, (s16_t)(-sizeof(struct eth_hdr)))) { + LWIP_ASSERT("Can't move over header in packet", 0); + pbuf_free(p); + p = NULL; + } + else + /* pass to IP layer */ + ip_input(p, netif); break; case ETHTYPE_ARP: diff --git a/src/core/ipv4/icmp.c b/src/core/ipv4/icmp.c index 248f2c6b..80e92c44 100644 --- a/src/core/ipv4/icmp.c +++ b/src/core/ipv4/icmp.c @@ -111,12 +111,14 @@ icmp_input(struct pbuf *p, struct netif *inp) /* increase number of echo replies attempted to send */ snmp_inc_icmpoutechoreps(); - pbuf_header(p, hlen); - ip_output_if(p, &(iphdr->src), IP_HDRINCL, - IPH_TTL(iphdr), 0, IP_PROTO_ICMP, inp); + if(pbuf_header(p, hlen)) + LWIP_ASSERT("Can't move over header in packet", 0); + else + ip_output_if(p, &(iphdr->src), IP_HDRINCL, + IPH_TTL(iphdr), 0, IP_PROTO_ICMP, inp); break; default: - LWIP_DEBUGF(ICMP_DEBUG, ("icmp_input: ICMP type %"S16_F" code %"S16_F" not supported.\n", (s16_t)type, (s16_t)code)); + LWIP_DEBUGF(ICMP_DEBUG, ("icmp_input: ICMP type %"S16_F" code %"S16_F" not supported.\n", (s16_t)type, (s16_t)code)); ICMP_STATS_INC(icmp.proterr); ICMP_STATS_INC(icmp.drop); } diff --git a/src/core/ipv6/ip6.c b/src/core/ipv6/ip6.c index 03037c81..ecb98653 100644 --- a/src/core/ipv6/ip6.c +++ b/src/core/ipv6/ip6.c @@ -220,8 +220,10 @@ ip_input(struct pbuf *p, struct netif *inp) { LWIP_DEBUGF("ip_input: p->len %"U16_F" p->tot_len %"U16_F"\n", p->len, p->tot_len);*/ #endif /* IP_DEBUG */ - - pbuf_header(p, -IP_HLEN); + if(pbuf_header(p, -IP_HLEN)) { + LWIP_ASSERT("Can't move over header in packet", 0); + return; + } switch (iphdr->nexthdr) { case IP_PROTO_UDP: @@ -266,7 +268,7 @@ ip_output_if (struct pbuf *p, struct ip_addr *src, struct ip_addr *dest, PERF_START; - printf("len %"U16_F" tot_len %"U16_F"\n", p->len, p->tot_len); + LWIP_DEBUGF(IP_DEBUG, ("len %"U16_F" tot_len %"U16_F"\n", p->len, p->tot_len)); if (pbuf_header(p, IP_HLEN)) { LWIP_DEBUGF(IP_DEBUG, ("ip_output: not enough room for IP header in pbuf\n")); #ifdef IP_STATS @@ -275,13 +277,13 @@ ip_output_if (struct pbuf *p, struct ip_addr *src, struct ip_addr *dest, return ERR_BUF; } - printf("len %"U16_F" tot_len %"U16_F"\n", p->len, p->tot_len); + LWIP_DEBUGF(IP_DEBUG, ("len %"U16_F" tot_len %"U16_F"\n", p->len, p->tot_len)); iphdr = p->payload; if (dest != IP_HDRINCL) { - printf("!IP_HDRLINCL\n"); + LWIP_DEBUGF(IP_DEBUG, ("!IP_HDRLINCL\n")); iphdr->hoplim = ttl; iphdr->nexthdr = proto; iphdr->len = htons(p->tot_len - IP_HLEN); diff --git a/src/core/pbuf.c b/src/core/pbuf.c index a9930098..d07ec31a 100644 --- a/src/core/pbuf.c +++ b/src/core/pbuf.c @@ -482,12 +482,15 @@ pbuf_header(struct pbuf *p, s16_t header_size_increment) LWIP_ASSERT("increment_magnitude <= p->len", increment_magnitude <= p->len); } else { increment_magnitude = header_size_increment; +#if 0 /* Can't assert these as some callers speculatively call + pbuf_header() to see if it's OK. Will return 1 below instead. */ /* Check that we've got the correct type of pbuf to work with */ LWIP_ASSERT("p->flags == PBUF_FLAG_RAM || p->flags == PBUF_FLAG_POOL", p->flags == PBUF_FLAG_RAM || p->flags == PBUF_FLAG_POOL); /* Check that we aren't going to move off the beginning of the pbuf */ LWIP_ASSERT("p->payload - increment_magnitude >= p + sizeof(struct pbuf)", (u8_t *)p->payload - increment_magnitude >= (u8_t *)p + sizeof(struct pbuf)); +#endif } flags = p->flags; @@ -520,6 +523,11 @@ pbuf_header(struct pbuf *p, s16_t header_size_increment) return 1; } } + else { + /* Unknown type */ + LWIP_ASSERT("bad pbuf flag type", 0); + return 1; + } /* modify pbuf length fields */ p->len += header_size_increment; p->tot_len += header_size_increment; diff --git a/src/core/raw.c b/src/core/raw.c index 30199804..dd3c09f2 100644 --- a/src/core/raw.c +++ b/src/core/raw.c @@ -220,7 +220,10 @@ raw_sendto(struct raw_pcb *pcb, struct pbuf *p, struct ip_addr *ipaddr) } else { /* first pbuf q equals given pbuf */ q = p; - pbuf_header(q, -IP_HLEN); + if(pbuf_header(q, -IP_HLEN)) { + LWIP_ASSERT("Can't restore header we just removed!", 0); + return ERR_MEM; + } } if ((netif = ip_route(ipaddr)) == NULL) { diff --git a/src/core/snmp/msg_in.c b/src/core/snmp/msg_in.c index 898c5604..95d27a42 100644 --- a/src/core/snmp/msg_in.c +++ b/src/core/snmp/msg_in.c @@ -803,7 +803,10 @@ snmp_recv(void *arg, struct udp_pcb *pcb, struct pbuf *p, struct ip_addr *addr, /* suppress unused argument warning */ if (arg); /* peek in the UDP header (goto IP payload) */ - pbuf_header(p, UDP_HLEN); + if(pbuf_header(p, UDP_HLEN)){ + LWIP_ASSERT("Can't move to UDP header", 0); + return; + } udphdr = p->payload; /* check if datagram is really directed at us (including broadcast requests) */ diff --git a/src/core/tcp_in.c b/src/core/tcp_in.c index a35e3895..5ab48682 100644 --- a/src/core/tcp_in.c +++ b/src/core/tcp_in.c @@ -149,7 +149,14 @@ tcp_input(struct pbuf *p, struct netif *inp) /* Move the payload pointer in the pbuf so that it points to the TCP data instead of the TCP header. */ hdrlen = TCPH_HDRLEN(tcphdr); - pbuf_header(p, -(hdrlen * 4)); + if(pbuf_header(p, -(hdrlen * 4))){ + /* drop short packets */ + LWIP_DEBUGF(TCP_INPUT_DEBUG, ("tcp_input: short packet\n")); + TCP_STATS_INC(tcp.lenerr); + TCP_STATS_INC(tcp.drop); + pbuf_free(p); + return; + } /* Convert fields in TCP header to host byte order. */ tcphdr->src = ntohs(tcphdr->src); @@ -900,9 +907,13 @@ tcp_receive(struct tcp_pcb *pcb) p->len = 0; p = p->next; } - pbuf_header(p, -off); + if(pbuf_header(p, -off)) + /* Do we need to cope with this failing? Assert for now */ + LWIP_ASSERT("pbuf_header failed", 0); } else { - pbuf_header(inseg.p, -off); + if(pbuf_header(inseg.p, -off)) + /* Do we need to cope with this failing? Assert for now */ + LWIP_ASSERT("pbuf_header failed", 0); } /* KJM following line changed to use p->payload rather than inseg->p->payload to fix bug #9076 */ diff --git a/src/core/tcp_out.c b/src/core/tcp_out.c index ff5ea242..c9bb57d0 100644 --- a/src/core/tcp_out.c +++ b/src/core/tcp_out.c @@ -312,7 +312,12 @@ tcp_enqueue(struct tcp_pcb *pcb, void *arg, u16_t len, /* fit within max seg size */ useg->len + queue->len <= pcb->mss) { /* Remove TCP header from first segment of our to-be-queued list */ - pbuf_header(queue->p, -TCP_HLEN); + if(pbuf_header(queue->p, -TCP_HLEN)) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + TCP_STATS_INC(tcp.err); + goto memerr; + } pbuf_cat(useg->p, queue->p); useg->len += queue->len; useg->next = queue->next; diff --git a/src/core/udp.c b/src/core/udp.c index 51daba7c..ed24c5ba 100644 --- a/src/core/udp.c +++ b/src/core/udp.c @@ -96,7 +96,7 @@ udp_input(struct pbuf *p, struct netif *inp) iphdr = p->payload; - if (p->tot_len < (IPH_HL(iphdr) * 4 + UDP_HLEN)) { + if (p->tot_len < (IPH_HL(iphdr) * 4 + UDP_HLEN) || pbuf_header(p, -(s16_t)(IPH_HL(iphdr) * 4))) { /* drop short packets */ LWIP_DEBUGF(UDP_DEBUG, ("udp_input: short UDP datagram (%"U16_F" bytes) discarded\n", p->tot_len)); @@ -107,8 +107,6 @@ udp_input(struct pbuf *p, struct netif *inp) goto end; } - pbuf_header(p, -(s16_t)(IPH_HL(iphdr) * 4)); - udphdr = (struct udp_hdr *)p->payload; LWIP_DEBUGF(UDP_DEBUG, ("udp_input: received datagram of length %"U16_F"\n", p->tot_len)); @@ -205,7 +203,14 @@ udp_input(struct pbuf *p, struct netif *inp) } #endif } - pbuf_header(p, -UDP_HLEN); + if(pbuf_header(p, -UDP_HLEN)) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + UDP_STATS_INC(udp.drop); + snmp_inc_udpinerrors(); + pbuf_free(p); + goto end; + } if (pcb != NULL) { snmp_inc_udpindatagrams(); /* callback */ @@ -319,7 +324,7 @@ udp_send(struct udp_pcb *pcb, struct pbuf *p) } /* not enough space to add an UDP header to first pbuf in given p chain? */ - if (p->flags == PBUF_FLAG_ROM || p->flags == PBUF_FLAG_REF || pbuf_header(p, UDP_HLEN)) { + if (pbuf_header(p, UDP_HLEN)) { /* allocate header in a seperate new pbuf */ q = pbuf_alloc(PBUF_IP, UDP_HLEN, PBUF_RAM); /* new header pbuf could not be allocated? */ diff --git a/src/netif/ppp/ppp.c b/src/netif/ppp/ppp.c index 5295f6a0..532845ce 100644 --- a/src/netif/ppp/ppp.c +++ b/src/netif/ppp/ppp.c @@ -1293,10 +1293,14 @@ static void pppInput(void *arg) u16_t protocol; int pd; - pd = ((struct pppInputHeader *)nb->payload)->unit; - protocol = ((struct pppInputHeader *)nb->payload)->proto; - - pbuf_header(nb, -(int)sizeof(struct pppInputHeader)); + pd = ((struct pppInputHeader *)nb->payload)->unit; + protocol = ((struct pppInputHeader *)nb->payload)->proto; + + if(pbuf_header(nb, -(int)sizeof(struct pppInputHeader))) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + return; + } #if LINK_STATS lwip_stats.link.recv++; @@ -1383,7 +1387,11 @@ static void pppInput(void *arg) /* No handler for this protocol so reject the packet. */ PPPDEBUG((LOG_INFO, "pppInput[%d]: rejecting unsupported proto 0x%04X len=%d\n", pd, protocol, nb->len)); - pbuf_header(nb, sizeof(protocol)); + if (pbuf_header(nb, sizeof(protocol))) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + goto drop; + } #if BYTE_ORDER == LITTLE_ENDIAN protocol = htons(protocol); memcpy(nb->payload, &protocol, sizeof(protocol)); diff --git a/src/netif/ppp/vj.c b/src/netif/ppp/vj.c index 0636ee11..a59c9620 100644 --- a/src/netif/ppp/vj.c +++ b/src/netif/ppp/vj.c @@ -371,13 +371,19 @@ u_int vj_compress_tcp( if (!comp->compressSlot || comp->last_xmit != cs->cs_id) { comp->last_xmit = cs->cs_id; hlen -= deltaS + 4; - pbuf_header(pb, -hlen); + if(pbuf_header(pb, -hlen)){ + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + } cp = (u_char *)pb->payload; *cp++ = changes | NEW_C; *cp++ = cs->cs_id; } else { hlen -= deltaS + 3; - pbuf_header(pb, -hlen); + if(pbuf_header(pb, -hlen)) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + } cp = (u_char *)pb->payload; *cp++ = changes; } @@ -573,8 +579,13 @@ int vj_uncompress_tcp( cs->cs_ip.ip_sum = (u_short)(~tmp); /* Remove the compressed header and prepend the uncompressed header. */ - pbuf_header(n0, -vjlen); - + if(pbuf_header(n0, -vjlen)) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + goto bad; + } + + if(MEM_ALIGN(n0->payload) != n0->payload) { struct pbuf *np, *q; u8_t *bufptr; @@ -586,7 +597,11 @@ int vj_uncompress_tcp( goto bad; } - pbuf_header(np, -cs->cs_hlen); + if(pbuf_header(np, -cs->cs_hlen)) { + /* Can we cope with this failing? Just assert for now */ + LWIP_ASSERT("pbuf_header failed\n", 0); + goto bad; + } bufptr = n0->payload; for(q = np; q != NULL; q = q->next) {