------- Comment #3 from meklund at cisco dot com 2006-01-17 15:36 ------- Subject: Re: feature request: generate a warning for sizeof on a pointer
Using the FreeBSD latest CVS pull on 10-Jan-06 (5.4 based), a build world was run with the below modifications to GCC. The output was then evaluated giving the following results. 22991 normal sizeof operations. 16564 sizeof(<typedef>) operations. 803 fu **x; sizeof(*x) operations. 396 fu *x; sizeof(x) operations. 40754 total The 396 "fu *x; sizeof(x)" type operations were then examined. Note that I am only concerning myself with the "fu *x; sizeof(x)" operations. I'm no longer suggesting issuing warning for "fu **x; sizeof(*x)", which are commonly used in the sizeof(m)/sizeof(*m) mentioned in my previous reply. The "fu *x; sizeof(x)" operations are broken down into the following categories: 1. (229) Calls within auto-generated files (mostly from cc_tools/gt*.[ch]) 2. (74) Calls involving DEPRECATED_REGISTER_GDBARCH_SWAP macro 3. (59) Calls like read(n, &i, sizeof(i)) or bcopy(x, &i, sizeof(i)) 4. (8) Misc seemingly correct calls. 5. (26) Likely bugs. The likely bugs are listed after the below patch. This means that 6.6% of what I want to display as optional warnings in well established code is actually bugs. If the auto-generated files are ignored, 15.6% were bugs. --- c-common.c 2006/01/10 15:42:38 1.2 +++ c-common.c 2006/01/11 15:43:06 @@ -5905,4 +5905,17 @@ error ("%s", string); } +FILE * +create_sizeof_stats_file(void) +{ + static FILE *sizeof_stats = NULL; + if (!sizeof_stats) { + char *sizeof_stats_fname = alloca(strlen(dump_base_name + 2 + + sizeof(".sizeof"))); + sprintf(sizeof_stats_fname, "%s.sizeof", dump_base_name); + sizeof_stats = fopen(sizeof_stats_fname, "w"); + } + return sizeof_stats; +} + #include "gt-c-common.h" --- c-common.h 2006/01/10 15:42:38 1.2 +++ c-common.h 2006/01/11 15:43:07 @@ -1330,4 +1330,6 @@ extern void pp_file_change (const struct line_map *); extern void pp_dir_change (cpp_reader *, const char *); +extern FILE * create_sizeof_stats_file(void); + #endif /* ! GCC_C_COMMON_H */ --- c-parse.in 2006/01/10 15:42:38 1.2 +++ c-parse.in 2006/01/11 16:43:30 @@ -518,9 +518,30 @@ if (TREE_CODE ($2) == COMPONENT_REF && DECL_C_BIT_FIELD (TREE_OPERAND ($2, 1))) error ("`sizeof' applied to a bit-field"); + static FILE *sizeof_stats = NULL; + sizeof_stats = create_sizeof_stats_file(); + if (TREE_CODE (TREE_TYPE ($2)) == POINTER_TYPE) { + if (TREE_CODE($2) == VAR_DECL) { + fprintf(sizeof_stats, + "%s: %d: MWE3:`sizeof' applied to a pointer" + " variable\n", input_filename, input_line); + } else { + fprintf(sizeof_stats, + "%s: %d: MWE2:`sizeof' reference\n", + input_filename, input_line); + } + } else { + fprintf(sizeof_stats, + "%s: %d: MWE0:`sizeof' reference\n", + input_filename, input_line); + } $$ = c_sizeof (TREE_TYPE ($2)); } | sizeof '(' typename ')' %prec HYPERUNARY { skip_evaluation--; + static FILE *sizeof_stats = NULL; + sizeof_stats = create_sizeof_stats_file(); + fprintf(sizeof_stats, "%s: %d: MWE1:`sizeof' reference\n", + input_filename, input_line); $$ = c_sizeof (groktypename ($3)); } | alignof unary_expr %prec UNARY { skip_evaluation--; --- /usr/src/gnu/usr.bin/binutils/readelf/../../../../contrib/binutils/binutils/readelf.c: 9569 Allocates too much memory. (Also, never frees it.) --- int cnt; /* Find the section header so that we get the size. */ while (sect->sh_type != SHT_MIPS_OPTIONS) ++sect; eopt = get_data (NULL, file, options_offset, sect->sh_size, _("options")); if (eopt) { * iopt = malloc ((sect->sh_size / sizeof (eopt)) * sizeof (*iopt)); if (iopt == NULL) { error (_("Out of memory")); return 0; } offset = cnt = 0; option = iopt; while (offset < sect->sh_size) --- /usr/src/gnu/usr.bin/gdb/libgdb/../../../../contrib/gdb/gdb/jv-valprint.c: 113 Assumes a 4 byte pointer. See FIXME. --- char *buf; buf = alloca (TARGET_PTR_BIT / HOST_CHAR_BIT); fputs_filtered (", ", stream); wrap_here (n_spaces (2)); if (i > 0) element = next_element; else { * read_memory (address, buf, sizeof (buf)); address += TARGET_PTR_BIT / HOST_CHAR_BIT; /* FIXME: cagney/2003-05-24: Bogus or what. It pulls a host sized pointer out of the target and then extracts that as an address (while assuming that the address is unsigned)! */ element = extract_unsigned_integer (buf, sizeof (buf)); } for (reps = 1; i + reps < length; reps++) { --- /usr/src/gnu/usr.bin/gdb/libgdb/../../../../contrib/gdb/gdb/jv-valprint.c: 119 FIXME says it all. --- if (i > 0) element = next_element; else { read_memory (address, buf, sizeof (buf)); address += TARGET_PTR_BIT / HOST_CHAR_BIT; /* FIXME: cagney/2003-05-24: Bogus or what. It pulls a host sized pointer out of the target and then extracts that as an address (while assuming that the address is unsigned)! */ * element = extract_unsigned_integer (buf, sizeof (buf)); } for (reps = 1; i + reps < length; reps++) { read_memory (address, buf, sizeof (buf)); address += TARGET_PTR_BIT / HOST_CHAR_BIT; /* FIXME: cagney/2003-05-24: Bogus or what. It pulls a host sized pointer out of the target and then extracts that as an address (while assuming that the address is unsigned)! */ --- /usr/src/gnu/usr.bin/gdb/libgdb/../../../../contrib/gdb/gdb/jv-valprint.c: 124 FIXME says it all. --- address += TARGET_PTR_BIT / HOST_CHAR_BIT; /* FIXME: cagney/2003-05-24: Bogus or what. It pulls a host sized pointer out of the target and then extracts that as an address (while assuming that the address is unsigned)! */ element = extract_unsigned_integer (buf, sizeof (buf)); } for (reps = 1; i + reps < length; reps++) { * read_memory (address, buf, sizeof (buf)); address += TARGET_PTR_BIT / HOST_CHAR_BIT; /* FIXME: cagney/2003-05-24: Bogus or what. It pulls a host sized pointer out of the target and then extracts that as an address (while assuming that the address is unsigned)! */ next_element = extract_unsigned_integer (buf, sizeof (buf)); if (next_element != element) break; } --- /usr/src/gnu/usr.bin/gdb/libgdb/../../../../contrib/gdb/gdb/jv-valprint.c: 130 FIXME says it all. --- } for (reps = 1; i + reps < length; reps++) { read_memory (address, buf, sizeof (buf)); address += TARGET_PTR_BIT / HOST_CHAR_BIT; /* FIXME: cagney/2003-05-24: Bogus or what. It pulls a host sized pointer out of the target and then extracts that as an address (while assuming that the address is unsigned)! */ * next_element = extract_unsigned_integer (buf, sizeof (buf)); if (next_element != element) break; } if (reps == 1) fprintf_filtered (stream, "%d: ", i); else fprintf_filtered (stream, "%d..%d: ", i, i + reps - 1); if (element == 0) --- /usr/src/lib/bind/bind/../../../contrib/bind9/lib/bind/irs/dns_ho.c: 263 Although the entire buffer pointed by 'q' eventually gets filled, this should be changed to either "q->next = NULL;" or "memset(q, 0, sizeof(*q))" --- if (init(this) == -1) return (NULL); q = memget(sizeof(*q)); if (q == NULL) { RES_SET_H_ERRNO(pvt->res, NETDB_INTERNAL); errno = ENOMEM; goto cleanup; } * memset(q, 0, sizeof(q)); switch (af) { case AF_INET: size = INADDRSZ; q->qclass = C_IN; q->qtype = T_A; q->answer = q->qbuf.buf; q->anslen = sizeof(q->qbuf); q->action = RESTGT_DOALWAYS; break; --- /usr/src/lib/bind/bind/../../../contrib/bind9/lib/bind/irs/dns_ho.c: 355 Although the entire buffer pointed by 'q' eventually gets filled, this should be changed to either "q->next = NULL;" or "memset(q, 0, sizeof(*q))" --- if (init(this) == -1) return (NULL); q = memget(sizeof(*q)); q2 = memget(sizeof(*q2)); if (q == NULL || q2 == NULL) { RES_SET_H_ERRNO(pvt->res, NETDB_INTERNAL); errno = ENOMEM; goto cleanup; } * memset(q, 0, sizeof(q)); memset(q2, 0, sizeof(q2)); if (af == AF_INET6 && len == IN6ADDRSZ && (!memcmp(uaddr, mapped, sizeof mapped) || (!memcmp(uaddr, tunnelled, sizeof tunnelled) && memcmp(&uaddr[sizeof tunnelled], v6local, sizeof(v6local))))) { /* Unmap. */ addr = (const char *)addr + sizeof mapped; uaddr += sizeof mapped; af = AF_INET; --- /usr/src/lib/bind/bind/../../../contrib/bind9/lib/bind/irs/dns_ho.c: 356 Although the entire buffer pointed by 'q2' eventually gets filled, this should be changed to either "q2->next = NULL;" or "memset(q2, 0, sizeof(*q2))" --- return (NULL); q = memget(sizeof(*q)); q2 = memget(sizeof(*q2)); if (q == NULL || q2 == NULL) { RES_SET_H_ERRNO(pvt->res, NETDB_INTERNAL); errno = ENOMEM; goto cleanup; } memset(q, 0, sizeof(q)); * memset(q2, 0, sizeof(q2)); if (af == AF_INET6 && len == IN6ADDRSZ && (!memcmp(uaddr, mapped, sizeof mapped) || (!memcmp(uaddr, tunnelled, sizeof tunnelled) && memcmp(&uaddr[sizeof tunnelled], v6local, sizeof(v6local))))) { /* Unmap. */ addr = (const char *)addr + sizeof mapped; uaddr += sizeof mapped; af = AF_INET; len = INADDRSZ; --- /usr/src/lib/bind/bind/../../../contrib/bind9/lib/bind/irs/dns_ho.c: 581 Although the entire buffer pointed by 'q' eventually gets filled, this should be changed to either "q->next = NULL;" or "memset(q, 0, sizeof(*q))" --- memset(&sentinel, 0, sizeof(sentinel)); cur = &sentinel; q = memget(sizeof(*q)); q2 = memget(sizeof(*q2)); if (q == NULL || q2 == NULL) { RES_SET_H_ERRNO(pvt->res, NETDB_INTERNAL); errno = ENOMEM; goto cleanup; } * memset(q, 0, sizeof(q2)); memset(q2, 0, sizeof(q2)); switch (pai->ai_family) { case AF_UNSPEC: /* prefer IPv6 */ q->qclass = C_IN; q->qtype = T_AAAA; q->answer = q->qbuf.buf; q->anslen = sizeof(q->qbuf); q->next = q2; --- /usr/src/lib/bind/bind/../../../contrib/bind9/lib/bind/irs/dns_ho.c: 582 Although the entire buffer pointed by 'q' eventually gets filled, this should be changed to either "q->next = NULL;" or "memset(q, 0, sizeof(*q))" --- cur = &sentinel; q = memget(sizeof(*q)); q2 = memget(sizeof(*q2)); if (q == NULL || q2 == NULL) { RES_SET_H_ERRNO(pvt->res, NETDB_INTERNAL); errno = ENOMEM; goto cleanup; } memset(q, 0, sizeof(q2)); * memset(q2, 0, sizeof(q2)); switch (pai->ai_family) { case AF_UNSPEC: /* prefer IPv6 */ q->qclass = C_IN; q->qtype = T_AAAA; q->answer = q->qbuf.buf; q->anslen = sizeof(q->qbuf); q->next = q2; q->action = RESTGT_DOALWAYS; --- /usr/src/lib/libopie/../../contrib/opie/libopie/readpass.c: 255 This is a bug on machines the have 8 byte pointers. --- if (*(e++) == *c) goto error; e = erase; while(*e) if (*(e++) == *c) { if (c <= buf) goto beep; if (flags & 1) * write(1, bsseq, sizeof(bsseq) - 1); c--; goto loop; } e = kill; while(*e) if (*(e++) == *c) { if (c <= buf) goto beep; --- /usr/src/lib/libopie/../../contrib/opie/libopie/readpass.c: 268 This is a bug on machines the have 8 byte pointers. --- } e = kill; while(*e) if (*(e++) == *c) { if (c <= buf) goto beep; if (flags & 1) while(c-- > buf) * write(1, bsseq, sizeof(bsseq) - 1); c = buf; goto loop; } if (c < end) { if (*c < 32) goto beep; if (flags & 1) write(1, c, 1); --- /usr/src/lib/libsdp/service.c: 227 This is likely looking at the memory right after the pdu. If that is true, the wrong memory is being read. sizeof(*pdu) should be used instead. --- pdu = (sdp_pdu_p) ss->rsp; pdu->tid = ntohs(pdu->tid); pdu->len = ntohs(pdu->len); if (pdu->pid != SDP_PDU_ERROR_RESPONSE || pdu->tid != ss->tid || pdu->len < 2 || pdu->len != len - sizeof(*pdu)) { ss->error = EIO; return (-1); } * error = (uint16_t) ss->rsp[sizeof(pdu)] << 8; error |= (uint16_t) ss->rsp[sizeof(pdu) + 1]; if (error != 0) { ss->error = EIO; return (-1); } return (len); } --- /usr/src/lib/libsdp/service.c: 228 This is likely looking at the memory right after the pdu. If that is true, the wrong memory is being read. sizeof(*pdu) should be used instead. --- pdu->tid = ntohs(pdu->tid); pdu->len = ntohs(pdu->len); if (pdu->pid != SDP_PDU_ERROR_RESPONSE || pdu->tid != ss->tid || pdu->len < 2 || pdu->len != len - sizeof(*pdu)) { ss->error = EIO; return (-1); } error = (uint16_t) ss->rsp[sizeof(pdu)] << 8; * error |= (uint16_t) ss->rsp[sizeof(pdu) + 1]; if (error != 0) { ss->error = EIO; return (-1); } return (len); } --- /usr/src/lib/libtelnet/../../contrib/telnet/libtelnet/sra.c: 306 pk_encode accesses pass in 4 byte increments. So the memset should have zeroed out the entire password. pk_encode could be reading random values. --- } break; case SRA_CONTINUE: if (passwd_sent) { passwd_sent = 0; printf("[ SRA login failed ]\r\n"); goto enc_user; } /* encode password */ * memset(pass,0,sizeof(pass)); telnet_gets("Password: ",pass,255,0); pk_encode(pass,xpass,&ck); /* send it off */ if (auth_debug_mode) printf("Sent KAB(P)\r\n"); if (!Data(ap, SRA_PASS, (void *)xpass, strlen(xpass))) { if (auth_debug_mode) printf("Not enough room\r\n"); return; } --- /usr/src/libexec/telnetd/../../contrib/telnet/telnetd/telnetd.c: 607 Although I don't know what size should be used for this strncpy, it is doubtful this is what was wanted. --- */ if (strncmp(first, terminaltype, sizeof(first)) == 0) break; /* * Get the terminal name one more time, so that * RFC1091 compliant telnets will cycle back to * the start of the list. */ _gettermname(); if (strncmp(first, terminaltype, sizeof(first)) != 0) { * (void) strncpy(terminaltype, first, sizeof(terminaltype)-1); terminaltype[sizeof(terminaltype)-1] = '\0'; } break; } } } } return(retval); } /* end of getterminaltype */ --- /usr/src/libexec/telnetd/../../contrib/telnet/telnetd/telnetd.c: 608 Although I don't know what size should be used for this strncpy, it is doubtful this is what was wanted. --- if (strncmp(first, terminaltype, sizeof(first)) == 0) break; /* * Get the terminal name one more time, so that * RFC1091 compliant telnets will cycle back to * the start of the list. */ _gettermname(); if (strncmp(first, terminaltype, sizeof(first)) != 0) { (void) strncpy(terminaltype, first, sizeof(terminaltype)-1); * terminaltype[sizeof(terminaltype)-1] = '\0'; } break; } } } } return(retval); } /* end of getterminaltype */ static void --- /usr/src/sbin/dhclient/common/../../../contrib/isc-dhcp/common/ctrace.c: 285 There is no documentation as to what the length referrs to. I'm assuming that it refers to the length of the seed characters. On a machine that has 8 byte pointers, the below would print an error. --- return; outseed = htonl (seed); trace_write_packet (ttype, sizeof outseed, (char *)&outseed, MDL); return; } void trace_seed_input (trace_type_t *ttype, unsigned length, char *buf) { u_int32_t *seed; * if (length != sizeof seed) { log_error ("trace_seed_input: wrong size (%d)", length); } seed = (u_int32_t *)buf; srandom (ntohl (*seed)); } void trace_seed_stop (trace_type_t *ttype) { } #endif /* TRACING */ --- /usr/src/sbin/dhclient/common/../../../contrib/isc-dhcp/common/icmp.c: 294 This is attempting to compensate for the memory consumed by ia. sizeof(*ia) should be used in the calculation. --- void trace_icmp_input_input (trace_type_t *ttype, unsigned length, char *buf) { struct iaddr *ia; unsigned len; u_int8_t *icbuf; ia = (struct iaddr *)buf; ia->len = ntohl(ia->len); icbuf = (u_int8_t *)(ia + 1); if (icmp_state -> icmp_handler) (*icmp_state -> icmp_handler) (*ia, icbuf, * (int)(length - sizeof ia)); } void trace_icmp_input_stop (trace_type_t *ttype) { } void trace_icmp_output_input (trace_type_t *ttype, unsigned length, char *buf) { struct icmp *icmp; struct iaddr ia; if (length != (sizeof (*icmp) + (sizeof ia))) { --- /usr/src/sbin/kldconfig/kldconfig.c: 273 This mallocs a pointer and then uses that memory as a stucture. Fun ensues. --- /* Break a string down into path components, store them into a queue */ static void parsepath(struct pathhead *pathq, char *path, int uniq) { char *p; struct pathentry *pe; while ((p = strsep(&path, ";")) != NULL) if (!uniq) { * if (((pe = malloc(sizeof(pe))) == NULL) || ((pe->path = strdup(p)) == NULL)) { errno = ENOMEM; err(1, "allocating path element"); } TAILQ_INSERT_TAIL(pathq, pe, next); } else { addpath(pathq, p, 1, 0); } } --- /usr/src/usr.bin/netstat/mbuf.c: 119 This mallocs(sizeof *mbstat), reads sizeof mbstat, and then access the structure members. The read should for sizeof(*mbstat) or mlen. --- struct mbtypenames *mp; bool *seen = NULL; mlen = sizeof *mbstat; if ((mbstat = malloc(mlen)) == NULL) { warn("malloc: cannot allocate memory for mbstat"); goto err; } if (mbaddr) { * if (kread(mbaddr, (char *)mbstat, sizeof mbstat)) goto err; if (kread(nmbcaddr, (char *)&nmbclusters, sizeof(int))) goto err; } else { mlen = sizeof *mbstat; if (sysctlbyname("kern.ipc.mbstat", mbstat, &mlen, NULL, 0) < 0) { warn("sysctl: retrieving mbstat"); goto err; } --- /usr/src/usr.bin/xlint/lint1/scan.l: 322 This should either memset the entire allocated buffer or do a sb->sb_name = NULL. --- allocsb(void) { sbuf_t *sb; if ((sb = sbfrlst) != NULL) { sbfrlst = sb->sb_nxt; } else { if ((sb = malloc(sizeof (sbuf_t))) == NULL) nomem(); } * (void)memset(sb, 0, sizeof (sb)); return (sb); } /* * Put a sbuf structure to the free list */ static void freesb(sbuf_t *sb) { --- /usr/src/usr.sbin/ppp/ncpaddr.c: 872 The sizeof res should be the length of the res buffer. --- if (len >= NCP_ASCIIBUFFERSIZE - 1) return res; switch (range->ncprange_family) { case AF_INET: if (range->ncprange_ip4width == -1) { /* A non-contiguous mask */ for (; len >= 3; res[len -= 2] = '\0') if (strcmp(res + len - 2, ".0")) break; * snprintf(res + len, sizeof res - len, "&0x%08lx", (unsigned long)ntohl(range->ncprange_ip4mask.s_addr)); } else if (range->ncprange_ip4width < 32) snprintf(res + len, sizeof res - len, "/%d", range->ncprange_ip4width); return res; #ifndef NOINET6 case AF_INET6: if (range->ncprange_ip6width != 128) snprintf(res + len, sizeof res - len, "/%d", range->ncprange_ip6width); --- /usr/src/usr.sbin/ppp/ncpaddr.c: 875 The sizeof res should be the length of the res buffer. --- switch (range->ncprange_family) { case AF_INET: if (range->ncprange_ip4width == -1) { /* A non-contiguous mask */ for (; len >= 3; res[len -= 2] = '\0') if (strcmp(res + len - 2, ".0")) break; snprintf(res + len, sizeof res - len, "&0x%08lx", (unsigned long)ntohl(range->ncprange_ip4mask.s_addr)); } else if (range->ncprange_ip4width < 32) * snprintf(res + len, sizeof res - len, "/%d", range->ncprange_ip4width); return res; #ifndef NOINET6 case AF_INET6: if (range->ncprange_ip6width != 128) snprintf(res + len, sizeof res - len, "/%d", range->ncprange_ip6width); return res; #endif --- /usr/src/usr.sbin/ppp/ncpaddr.c: 882 The sizeof res should be the length of the res buffer. --- snprintf(res + len, sizeof res - len, "&0x%08lx", (unsigned long)ntohl(range->ncprange_ip4mask.s_addr)); } else if (range->ncprange_ip4width < 32) snprintf(res + len, sizeof res - len, "/%d", range->ncprange_ip4width); return res; #ifndef NOINET6 case AF_INET6: if (range->ncprange_ip6width != 128) * snprintf(res + len, sizeof res - len, "/%d", range->ncprange_ip6width); return res; #endif } return "<AF_UNSPEC>"; } #ifndef NOINET6 int --- /usr/src/usr.sbin/tcpdump/tcpdump/../../../contrib/tcpdump/print-ospf6.c: 351 This is likely checking if the memory goes beyond the end of available. They probably want to do "lsap + 1" instead of "lsap + sizeof(lsapp)". Also, unless k becomes nonzero, I don't know how they get outof the loop. --- printf(" %s", ipaddr_string(ap)); ++ap; } break; case LS_TYPE_INTER_AP | LS_SCOPE_AREA: TCHECK(lsap->lsa_un.un_inter_ap.inter_ap_metric); printf(" metric %u", EXTRACT_32BITS(&lsap->lsa_un.un_inter_ap.inter_ap_metric) & SLA_MASK_METRIC); lsapp = lsap->lsa_un.un_inter_ap.inter_ap_prefix; * while (lsapp + sizeof(lsapp) <= (struct lsa_prefix *)ls_end) { k = ospf6_print_lsaprefix(lsapp); if (k) goto trunc; lsapp = (struct lsa_prefix *)(((u_char *)lsapp) + k); } break; case LS_SCOPE_AS | LS_TYPE_ASE: TCHECK(lsap->lsa_un.un_asla.asla_metric); flags32 = EXTRACT_32BITS(&lsap->lsa_un.un_asla.asla_metric); ospf6_print_bits(ospf6_asla_flag_bits, flags32); -- http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25702