From 7294cb080b9f3b049f786729fddf7fc2f6d4b05d Mon Sep 17 00:00:00 2001 From: kieranm Date: Wed, 21 Mar 2007 12:55:00 +0000 Subject: [PATCH] * 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. --- CHANGELOG | 7 ++++++- src/api/tcpip.c | 11 ++++++++--- src/core/ipv4/icmp.c | 10 ++++++---- src/core/ipv6/ip6.c | 12 +++++++----- src/core/pbuf.c | 8 ++++++++ src/core/raw.c | 5 ++++- src/core/snmp/msg_in.c | 5 ++++- src/core/tcp_in.c | 17 ++++++++++++++--- src/core/tcp_out.c | 7 ++++++- src/core/udp.c | 15 ++++++++++----- src/netif/ppp/ppp.c | 18 +++++++++++++----- src/netif/ppp/vj.c | 25 ++++++++++++++++++++----- 12 files changed, 106 insertions(+), 34 deletions(-) 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) {