One common mistake in C is to do sizeof(x) where x is a pointer and what was really wanted was sizeof(*x). Similar to requiring parentheses around assignments in conditionals, I'd like an optional warning to be added when sizeof is done on a variable that is a pointer. For example, in the code below, the first sizeof would give a warning. To prevent this warning, sizeof(char *) can be used instead. void fu (void) { char *x; printf(" x %d\n", sizeof(x)); printf("char * %d\n", sizeof(char *)); printf("char * %d\n", sizeof("hello world")); } I've done a proof of concept for gcc 3.3 as below. I haven't done extensive testing and I didn't make the warning optional. *** c-parse.y 2005/12/05 22:19:42 1.1 --- c-parse.y 2005/12/05 22:54:43 *************** *** 494,502 **** --- 494,506 ---- if (TREE_CODE ($2) == COMPONENT_REF && DECL_C_BIT_FIELD (TREE_OPERAND ($2, 1))) error ("`sizeof' applied to a bit-field"); + warning("`sizeof' reference #1"); + if (TREE_CODE (TREE_TYPE ($2)) == POINTER_TYPE) + warning("`sizeof' applied to a pointer variable"); $$ = c_sizeof (TREE_TYPE ($2)); } | sizeof '(' typename ')' %prec HYPERUNARY { skip_evaluation--; + warning("`sizeof' reference #2"); $$ = c_sizeof (groktypename ($3)); } | alignof unary_expr %prec UNARY { skip_evaluation--;
Actually people use sizeof(x) all the time to mean the correct thing, for an example: memcpy(&x, y, sizeof(x));
Subject: Re: feature request: generate a warning for sizeof on a pointer On Fri, Jan 06, 2006 at 10:12:55PM -0000, pinskia at gcc dot gnu dot org wrote: > Actually people use sizeof(x) all the time to mean the correct thing, for an > example: memcpy(&x, y, sizeof(x)); True, and that is why I'd like to make it an optional warning. People would be up in arms if it weren't optional. But, for people that want to avoid this easily missed problem, they could live with memcpy(&x, y, sizeof(xtype *)) I have seen one instance where people would consider it annoying: char *m[] = { "this", "is", "bothersome", "to", "some" }; int m_items = sizeof(m) / sizeof(*m); but once again, the avoidance of having unexpectedly short lengths would override the annoyance for many. How about I apply my patch and do a large build like BSD "make world" and come back with a listing of how prevalent the above is? --mark
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);
So what the BSD people said about this? Did they agree with your assessment? How many of those 26 likely bugs were considered "real" bugs by them? It really seems too noisy and with no clear way to avoid or workaround an invalid warning.
Subject: Re: feature request: generate a warning for sizeof on a pointer Hi Manu, This is in regards to your Comments for the gcc feature enhancement request, http://gcc.gnu.org/bugzilla/show_bug.cgi?id=25702. On Wed, Nov 28, 2007 at 05:34:35PM -0000, manu at gcc dot gnu dot org wrote: > > ------- Comment #4 from manu at gcc dot gnu dot org 2007-11-28 17:34 ------- > So what the BSD people said about this? Did they agree with your assessment? > How many of those 26 likely bugs were considered "real" bugs by them? When I contacted Matt Dillon, he said all of them except two were definitely bugs. The other two were not in the Dragonfly code base, so he didn't comment on those. The 24 definite bugs have been corrected in Dragonfly. Paul Vixie corrected the bind bugs in the BIND8 sources and said thought this check would be a great compiler feature. I don't know if the bugs have been fixed in FreeBSD. I contacted security-officer@freebsd.org to make sure there were no security problems cause by these bugs and got no response. I didn't pursue the matter further after that. > > It really seems too noisy and with no clear way to avoid or workaround an > invalid warning. > This should be implemented in such a way that 'sizeof(ptr)' gets warning, but 'sizeof(char *)' doesn't. --mark
Actually I use sizeof all the time on pointers so I don't think this is useful. In fact it falls down with meta programming. That is: #define bitcase(type, a) ({typeof (a) b = a; type c; int notthesamesize[sizeof(a) == sizeof(type)]; memcpy(&c, &a, sizeof(a)); c;}) And then type is some pointer type.
I believe this should be closed as WONTFIX. Warnings exist to indicate things in the program that are almost certainly wrong. In this case, the only way to really avoid false positives is to look at the context of the sizeof, which is more something a static checker would do.
I encounter this mistake regularly too. But you have shown examples why shouldn't we need warning for that. But I have a different idea. The sizeof operator is usually used to pass buffer sizes to a function, along with pointer to the buffer itself. So when you have a function call like: dumpbuf(a, sizeof(a)) It's almost certainly wrong if `a` is a pointer, isn't it? So basically the rule would be: if x and sizeof(x) is in the same expression and x is a pointer, issue a warning.
See -Wsizeof-pointer-memaccess warning added in GCC 4.8. Currently it only warns about a handful of builtin functions there is no attribute to annotate user functions if they want similar behavior, but at least for various memory/string functions, s*printf etc. you'll get warnings.
I think -Wsizeof-pointer-memaccess should be enough for this PR be closed now.
-Wsizeof-pointer-memaccess is definitely a good targeted fix and probably hits a majority of what I've seen. I'm good with this being resolved.
PR 71219 requests a new warning for: struct S* p = (struct S*)malloc(sizeof(p)); which would solve some of the other bugs shown in comment 3.
If we add such a warning, IMNSHO it shouldn't be in -Wall, because sizeof on pointer is a perfectly valid thing, which is used in huge amounts of code, there is really nothing wrong on it.
See PR 71219 and the TC it links to. The suggestion is to warn for (T*)malloc(n) where n < sizeof(T) i.e. when there is enough context to infer that what was intended was malloc(sizeof(T)). The occurrence of sizeof(p) in the example is not essential to the point, it's just a common way that the bug happens.
Even (T*)malloc(n) where n < sizeof(T) is quite common, e.g. in GCC itself (well, we don't usually use malloc but some ggc_alloc*) - e.g. if T is a union and the code wants to allocate memory just for one of the union members, or if there is some struct at the beginning and user wants to allocate memory just for that initial struct rather than for the whole object.