Re: Removing -Wno-format from kernel makefiles, V2

2013-07-03 Thread Alexander Bluhm
On Wed, Jul 03, 2013 at 01:46:11PM +0200, Stefan Fritsch wrote:
 which the patches are applied does not matter. Therefore I thought that 
 people could either review by patch from the above directory or by source 
 file from the combined patch, just as they are comfortable. I would then 
 commit the patches bit by bit. Or is there a better suggestion?

For me it is easiest to review on a directory/file basis.  Also
partial commit should be easy that way.

Starting with netinet I found this:

-   panic(ip_output: tag of length %d (should be %d,
+   panic(ip_output: tag of length %d (should be %zd,
mtag-m_tag_len, sizeof (struct tdb_ident));

m_tag_len and sizeof are unsigned.  We should fix that with %u and
%zu also.  And the same code is in ip6_output and ip6_forward.

bluhm



Re: Removing -Wno-format from kernel makefiles, V2

2013-07-03 Thread Theo de Raadt
Please always include diffs in the emails; don't force us to go
to a browser.



Removing -Wno-format from kernel makefiles, 1/16

2013-07-03 Thread Stefan Fritsch
add support for %td for ptrdiff_t in kernel

this also adds support in gcc 4.x kprintf
---
 gnu/gcc/gcc/c-format.c |7 ---
 sys/kern/subr_prf.c|6 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git gnu/gcc/gcc/c-format.c gnu/gcc/gcc/c-format.c
index b9eecee..1b1734b 100644
--- gnu/gcc/gcc/c-format.c
+++ gnu/gcc/gcc/c-format.c
@@ -325,6 +325,7 @@ static const format_length_info kprintf_length_specs[] =
   { l, FMT_LEN_l, STD_C89, ll, FMT_LEN_ll, STD_C9L },
   { q, FMT_LEN_ll, STD_EXT, NULL, 0, 0 },
   { z, FMT_LEN_z, STD_C99, NULL, 0, 0 },
+  { t, FMT_LEN_t, STD_C99, NULL, 0, 0 },
   { NULL, 0, 0, NULL, 0, 0 }
 };
 
@@ -552,9 +553,9 @@ static const format_char_info asm_fprintf_char_table[] =
 static const format_char_info kprint_char_table[] = 
 { 
   /* C89 conversion specifiers.  */ 
-  { di,  0, STD_C89, { T89_I,   BADLEN, T89_S,   T89_L,   T9L_LL,  BADLEN,  
T99_SST, BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0 +'I, i, NULL }, 
-  { oxX, 0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN,  
T99_ST,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0#,i, NULL }, 
-  { u,   0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN,  
T99_ST,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0'I,   i, NULL }, 
+  { di,  0, STD_C89, { T89_I,   BADLEN, T89_S,   T89_L,   T9L_LL,  BADLEN,  
T99_SST, T99_PD,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0 +'I, i, NULL }, 
+  { oxX, 0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN,  
T99_ST,  T99_UPD,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0#,i, NULL 
}, 
+  { u,   0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN,  
T99_ST,  T99_UPD,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0'I,   i, NULL 
}, 
   { c,   0, STD_C89, { T89_I,   BADLEN, BADLEN,  BADLEN,  BADLEN,  BADLEN,  
BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -w,   , NULL }, 
   { s,   1, STD_C89, { T89_C,   BADLEN, BADLEN,  BADLEN,  BADLEN,  BADLEN,  
BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp,  cR, NULL 
}, 
   { p,   1, STD_C89, { T89_V,   BADLEN, BADLEN,  T89_UL,  T9L_LL,  BADLEN,  
BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0, c, NULL }, 
diff --git sys/kern/subr_prf.c sys/kern/subr_prf.c
index 768d164..c940141 100644
--- sys/kern/subr_prf.c
+++ sys/kern/subr_prf.c
@@ -842,6 +842,12 @@ reswitch:  switch (ch) {
size = 1;
sign = '\0';
break;
+   case 't':
+   {
+   /* assume ptrdiff_t is long */
+   CTASSERT(sizeof(fmt - fmt0) == sizeof(long));
+   }
+   /* FALLTHROUGH */
case 'D':
flags |= LONGINT;
/*FALLTHROUGH*/



Re: Removing -Wno-format from kernel makefiles, 1/16

2013-07-03 Thread Mark Kettenis
 Date: Wed, 3 Jul 2013 16:35:24 +0200 (CEST)
 From: Stefan Fritsch s...@sfritsch.de
 
 add support for %td for ptrdiff_t in kernel
 
 this also adds support in gcc 4.x kprintf

I'm on the fence about the CTASSERT here.  If we ever support a code
model that's not ILP32 or LP64, we need a major overhaul of the code
base.  So I don't think it adds real value.

so ok kettenis@ with or without that CTASSERT.

 ---
  gnu/gcc/gcc/c-format.c |7 ---
  sys/kern/subr_prf.c|6 ++
  2 files changed, 10 insertions(+), 3 deletions(-)
 
 diff --git gnu/gcc/gcc/c-format.c gnu/gcc/gcc/c-format.c
 index b9eecee..1b1734b 100644
 --- gnu/gcc/gcc/c-format.c
 +++ gnu/gcc/gcc/c-format.c
 @@ -325,6 +325,7 @@ static const format_length_info kprintf_length_specs[] =
{ l, FMT_LEN_l, STD_C89, ll, FMT_LEN_ll, STD_C9L },
{ q, FMT_LEN_ll, STD_EXT, NULL, 0, 0 },
{ z, FMT_LEN_z, STD_C99, NULL, 0, 0 },
 +  { t, FMT_LEN_t, STD_C99, NULL, 0, 0 },
{ NULL, 0, 0, NULL, 0, 0 }
  };
  
 @@ -552,9 +553,9 @@ static const format_char_info asm_fprintf_char_table[] =
  static const format_char_info kprint_char_table[] = 
  { 
/* C89 conversion specifiers.  */ 
 -  { di,  0, STD_C89, { T89_I,   BADLEN, T89_S,   T89_L,   T9L_LL,  BADLEN, 
  T99_SST, BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0 +'I, i, NULL 
 }, 
 -  { oxX, 0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN, 
  T99_ST,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0#,i, NULL 
 }, 
 -  { u,   0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN, 
  T99_ST,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0'I,   i, NULL 
 }, 
 +  { di,  0, STD_C89, { T89_I,   BADLEN, T89_S,   T89_L,   T9L_LL,  BADLEN, 
  T99_SST, T99_PD,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0 +'I, i, NULL 
 }, 
 +  { oxX, 0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN, 
  T99_ST,  T99_UPD,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0#,i, 
 NULL }, 
 +  { u,   0, STD_C89, { T89_UI,  BADLEN, T89_US,  T89_UL,  T9L_ULL, BADLEN, 
  T99_ST,  T99_UPD,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0'I,   i, 
 NULL }, 
{ c,   0, STD_C89, { T89_I,   BADLEN, BADLEN,  BADLEN,  BADLEN,  BADLEN, 
  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -w,   , NULL 
 }, 
{ s,   1, STD_C89, { T89_C,   BADLEN, BADLEN,  BADLEN,  BADLEN,  BADLEN, 
  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp,  cR, 
 NULL }, 
{ p,   1, STD_C89, { T89_V,   BADLEN, BADLEN,  T89_UL,  T9L_LL,  BADLEN, 
  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN }, -wp0, c, NULL 
 }, 
 diff --git sys/kern/subr_prf.c sys/kern/subr_prf.c
 index 768d164..c940141 100644
 --- sys/kern/subr_prf.c
 +++ sys/kern/subr_prf.c
 @@ -842,6 +842,12 @@ reswitch:switch (ch) {
   size = 1;
   sign = '\0';
   break;
 + case 't':
 + {
 + /* assume ptrdiff_t is long */
 + CTASSERT(sizeof(fmt - fmt0) == sizeof(long));
 + }
 + /* FALLTHROUGH */
   case 'D':
   flags |= LONGINT;
   /*FALLTHROUGH*/
 
 



Re: Removing -Wno-format from kernel makefiles, 2/16

2013-07-03 Thread Mark Kettenis
 Date: Wed, 3 Jul 2013 16:40:17 +0200 (CEST)
 From: Stefan Fritsch s...@sfritsch.de
 
 don't pass empty format string in subr_disk.c
 
 this is necessary to enable -Wformat or -Wno-error=format

Don't think this one makes much sense.  Better to just do:

  log(pri, %s, );

and keep the rest of the code as it is.  Not sure what the static
const trickery was necessary for though.

 ---
  sys/kern/subr_disk.c |   11 ++-
  1 files changed, 6 insertions(+), 5 deletions(-)
 
 diff --git sys/kern/subr_disk.c sys/kern/subr_disk.c
 index 4b500c1..2b1036a 100644
 --- sys/kern/subr_disk.c
 +++ sys/kern/subr_disk.c
 @@ -761,13 +761,14 @@ diskerr(struct buf *bp, char *dname, char *what, int 
 pri, int blkdone,
   daddr_t sn;
  
   if (pri != LOG_PRINTF) {
 - static const char fmt[] = ;
 - log(pri, fmt);
 + log(pri, %s%d%c: %s %sing fsbn , dname, unit, partname, what,
 + bp-b_flags  B_READ ? read : writ);
   pr = addlog;
 - } else
 + } else {
 + printf(%s%d%c: %s %sing fsbn , dname, unit, partname, what,
 + bp-b_flags  B_READ ? read : writ);
   pr = printf;
 - (*pr)(%s%d%c: %s %sing fsbn , dname, unit, partname, what,
 - bp-b_flags  B_READ ? read : writ);
 + }
   sn = bp-b_blkno;
   if (bp-b_bcount = DEV_BSIZE)
   (*pr)(%lld, sn);
 -- 
 1.7.6
 
 
 



Re: Removing -Wno-format from kernel makefiles, 4/16

2013-07-03 Thread Mark Kettenis
 Date: Wed, 3 Jul 2013 16:55:46 +0200 (CEST)
 From: Stefan Fritsch s...@sfritsch.de
 
 format string fixes: long
 
 ---
  sys/arch/i386/i386/esm.c |2 +-
  sys/kern/kern_descrip.c  |4 ++--
  2 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git sys/arch/i386/i386/esm.c sys/arch/i386/i386/esm.c
 index c90b2c4..3dff69e 100644
 --- sys/arch/i386/i386/esm.c
 +++ sys/arch/i386/i386/esm.c
 @@ -880,7 +880,7 @@ esm_make_sensors(struct esm_softc *sc, struct esm_devmap 
 *devmap,
   }
  
   for (j = 0; j  nsensors; j++) {
 - snprintf(s[j].desc, sizeof(s[j].desc), %s %d,
 + snprintf(s[j].desc, sizeof(s[j].desc), %s %ld,
   sensor_map[i].name, sensor_map[i].arg + j);
   }
   break;

Looking at this one, it makes more sense to make the arg member of
struct esm_sensor_map an int.  That will result in some space
savings if we'd ever bring this driver to amd64.

 diff --git sys/kern/kern_descrip.c sys/kern/kern_descrip.c
 index 50eda54..bc63a86 100644
 --- sys/kern/kern_descrip.c
 +++ sys/kern/kern_descrip.c
 @@ -1061,7 +1061,7 @@ closef(struct file *fp, struct proc *p)
  
  #ifdef DIAGNOSTIC
   if (fp-f_count  2)
 - panic(closef: count (%d)  2, fp-f_count);
 + panic(closef: count (%ld)  2, fp-f_count);
  #endif
   fp-f_count--;
  
 @@ -1097,7 +1097,7 @@ fdrop(struct file *fp, struct proc *p)
  
  #ifdef DIAGNOSTIC
   if (fp-f_count != 0)
 - panic(fdrop: count (%d) != 0, fp-f_count);
 + panic(fdrop: count (%ld) != 0, fp-f_count);
  #endif
  
   if (fp-f_ops)

ok kettenis@



libc malloc poison

2013-07-03 Thread Ted Unangst
change the junking to be word size. maybe later we can also change the
values to be random or something.

Index: stdlib/malloc.c
===
RCS file: /cvs/src/lib/libc/stdlib/malloc.c,v
retrieving revision 1.149
diff -u -p -r1.149 malloc.c
--- stdlib/malloc.c 22 Dec 2012 07:32:17 -  1.149
+++ stdlib/malloc.c 3 Jul 2013 19:52:27 -
@@ -80,14 +80,6 @@
 
 #define PAGEROUND(x)  (((x) + (MALLOC_PAGEMASK))  ~MALLOC_PAGEMASK)
 
-/*
- * What to use for Junk.  This is the byte value we use to fill with
- * when the 'J' option is enabled. Use SOME_JUNK right after alloc,
- * and SOME_FREEJUNK right before free.
- */
-#define SOME_JUNK  0xd0/* as in Duh :-) */
-#define SOME_FREEJUNK  0xdf
-
 #define MMAP(sz)   mmap(NULL, (size_t)(sz), PROT_READ | PROT_WRITE, \
 MAP_ANON | MAP_PRIVATE, -1, (off_t) 0)
 
@@ -237,6 +229,18 @@ hash(void *p)
 }
 
 static void
+poison(void *p, size_t len)
+{
+   int *ip = p;
+   size_t i;
+   int pval = 0xd0d0caca;
+
+   len = len / 4;
+   for (i = 0; i  len; i++)
+   ip[i] = pval;
+}
+
+static void
 wrterror(char *msg, void *p)
 {
char*q =  error: ;
@@ -419,7 +423,7 @@ map(struct dir_info *d, size_t sz, int z
memset(p, 0, sz);
else if (mopts.malloc_junk 
mopts.malloc_freeunmap)
-   memset(p, SOME_FREEJUNK, sz);
+   poison(p, sz);
return p;
} else if (r-size  psz)
big = r;
@@ -437,7 +441,7 @@ map(struct dir_info *d, size_t sz, int z
if (zero_fill)
memset(p, 0, sz);
else if (mopts.malloc_junk  mopts.malloc_freeunmap)
-   memset(p, SOME_FREEJUNK, sz);
+   poison(p, sz);
return p;
}
p = MMAP(sz);
@@ -975,7 +979,7 @@ malloc_bytes(struct dir_info *d, size_t 
k = bp-shift;
 
if (mopts.malloc_junk  bp-size  0)
-   memset((char *)bp-page + k, SOME_JUNK, bp-size);
+   poison((char *)bp-page + k, bp-size);
return ((char *)bp-page + k);
 }
 
@@ -1073,7 +1077,7 @@ omalloc(size_t sz, int zero_fill, void *
MALLOC_LEEWAY) {
/* fill whole allocation */
if (mopts.malloc_junk)
-   memset(p, SOME_JUNK, psz - mopts.malloc_guard);
+   poison(p, psz - mopts.malloc_guard);
/* shift towards the end */
p = ((char *)p) + ((MALLOC_PAGESIZE - MALLOC_LEEWAY -
(sz - mopts.malloc_guard))  ~(MALLOC_MINSIZE-1));
@@ -1083,11 +1087,10 @@ omalloc(size_t sz, int zero_fill, void *
} else {
if (mopts.malloc_junk) {
if (zero_fill)
-   memset((char *)p + sz - 
mopts.malloc_guard,
-   SOME_JUNK, psz - sz);
+   poison((char *)p + sz -
+   mopts.malloc_guard, psz - sz);
else
-   memset(p, SOME_JUNK,
-   psz - mopts.malloc_guard);
+   poison(p, psz - mopts.malloc_guard);
}
}
 
@@ -1202,8 +1205,7 @@ ofree(void *p)
malloc_guarded -= mopts.malloc_guard;
}
if (mopts.malloc_junk  !mopts.malloc_freeunmap)
-   memset(p, SOME_FREEJUNK,
-   PAGEROUND(sz) - mopts.malloc_guard);
+   poison(p, PAGEROUND(sz) - mopts.malloc_guard);
unmap(g_pool, p, PAGEROUND(sz));
delete(g_pool, r);
} else {
@@ -1211,7 +1213,7 @@ ofree(void *p)
int i;
 
if (mopts.malloc_junk  sz  0)
-   memset(p, SOME_FREEJUNK, sz);
+   poison(p, sz);
if (!mopts.malloc_freenow) {
i = getrnibble();
tmp = p;
@@ -1308,7 +1310,7 @@ orealloc(void *p, size_t newsz, void *f)
if (q == hint) {
malloc_used += needed;
if (mopts.malloc_junk)
-   memset(q, SOME_JUNK, needed);
+   poison(q, needed);
r-size = newsz;
STATS_SETF(r, f);
 

Re: libc malloc poison

2013-07-03 Thread Theo de Raadt
 +   int pval = 0xd0d0caca;

Can you explain the choice of this?

There are arguments to make this MI; other arguments to make it MD;
and other arguments to introduce a bit of randomness.

I'd like to know which arguments you have



Re: libc malloc poison

2013-07-03 Thread Ted Unangst
On Wed, Jul 03, 2013 at 17:21, Theo de Raadt wrote:
 +   int pval = 0xd0d0caca;
 
 Can you explain the choice of this?

I thought it sounded clever.

 There are arguments to make this MI; other arguments to make it MD;
 and other arguments to introduce a bit of randomness.
 
 I'd like to know which arguments you have

Since libc doesn't do free list integrity checking, I'm currently
leaning towards a random value. (even with random, we could still
check that all words of a free chunk are the same.)

Somebody also noticed that we don't have separate values for allocated
and freed memory. I suppose this makes debugging harder since you
can't obviously identify freed memory? I lean towards prioritizing
finding more bugs, which implies we need more variability, since any
one value may allow a program to work where a different value would
not.