* [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp
@ 2015-08-21 14:01 rjohnston
2015-08-21 14:01 ` [PATCH 1/2] xfsdump: use 64bit local variables in inode.c rjohnston
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: rjohnston @ 2015-08-21 14:01 UTC (permalink / raw)
To: xfs
The memset in cb_add_inogrp will segfault when the index oldsize
overflows. In cb_add_inogrp(), the temp variables used in
calculating the new i2gmap segment offset should be int64 instead
of intgen_t (int32).
A second bug also occurs because we already compensate for the
length of each item in oldsize so are 32bit wrap becomes a 40bit
wrap.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] xfsdump: use 64bit local variables in inode.c 2015-08-21 14:01 [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp rjohnston @ 2015-08-21 14:01 ` rjohnston 2015-08-21 17:03 ` Eric Sandeen 2015-08-21 18:22 ` Eric Sandeen 2015-08-21 14:01 ` [PATCH 2/2] xfsdump: don't do pointer math twice rjohnston 2015-08-21 15:47 ` [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp Eric Sandeen 2 siblings, 2 replies; 13+ messages in thread From: rjohnston @ 2015-08-21 14:01 UTC (permalink / raw) To: xfs [-- Attachment #1: xfsdump-use-64bit-local-variables-in-inode-c.patch --] [-- Type: text/plain, Size: 4212 bytes --] The memset in cb_add_inogrp will segfault when the index oldsize overflows. In cb_add_inogrp(), the temp variables used in calculating the new i2gmap segment offset should be int64 instead of intgen_t (int32). Fix this: a. simplify the calculation of oldsize by moving it before hnkmaplen is incremented. b. change the index variables int64 to prevent overflow. Signed-off-by: Rich Johnston <rjohnston@sgi.com> --- dump/inomap.c | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) Index: b/dump/inomap.c =================================================================== --- a/dump/inomap.c +++ b/dump/inomap.c @@ -71,7 +71,7 @@ static intgen_t cb_context( bool_t last, drange_t *, startpt_t *, size_t, - intgen_t, + int64_t, bool_t, bool_t *); static void cb_context_free( void ); @@ -96,7 +96,7 @@ static off64_t estimate_dump_space( xfs_ /* inomap primitives */ -static intgen_t inomap_init( intgen_t igrpcnt ); +static intgen_t inomap_init( int64_t igrpcnt ); static void inomap_add( void *, xfs_ino_t ino, gen_t gen, intgen_t ); static intgen_t inomap_set_state( void *, xfs_ino_t ino, intgen_t ); static void inomap_set_gen(void *, xfs_ino_t, gen_t ); @@ -160,7 +160,7 @@ inomap_build( jdm_fshandle_t *fshandlep, xfs_bstat_t *bstatbufp; size_t bstatbuflen; bool_t pruneneeded = BOOL_FALSE; - intgen_t igrpcnt = 0; + int64_t igrpcnt = 0; intgen_t stat; intgen_t rval; @@ -449,7 +449,7 @@ cb_context( bool_t last, drange_t *resumerangep, startpt_t *startptp, size_t startptcnt, - intgen_t igrpcnt, + int64_t igrpcnt, bool_t skip_unchanged_dirs, bool_t *pruneneededp ) { @@ -949,14 +949,14 @@ struct i2gseg { typedef struct i2gseg i2gseg_t; typedef struct seg_addr { - intgen_t hnkoff; - intgen_t segoff; - intgen_t inooff; + int64_t hnkoff; + int64_t segoff; + int64_t inooff; } seg_addr_t; static struct inomap { hnk_t *hnkmap; - intgen_t hnkmaplen; + int64_t hnkmaplen; i2gseg_t *i2gmap; seg_addr_t lastseg; } inomap; @@ -1040,7 +1040,7 @@ SEG_GET_BITS( seg_t *segp, xfs_ino_t ino /* context for inomap construction - initialized by map_init */ static intgen_t -inomap_init( intgen_t igrpcnt ) +inomap_init( int64_t igrpcnt ) { ASSERT( sizeof( hnk_t ) == HNKSZ ); @@ -1066,7 +1066,7 @@ inomap_getsz( void ) static inline bool_t inomap_validaddr( seg_addr_t *addrp ) { - intgen_t maxseg; + int64_t maxseg; if ( addrp->hnkoff < 0 || addrp->hnkoff > inomap.lastseg.hnkoff ) return BOOL_FALSE; @@ -1093,13 +1093,13 @@ inomap_addr2seg( seg_addr_t *addrp ) return &hunkp->seg[addrp->segoff]; } -static inline intgen_t +static inline int64_t inomap_addr2segix( seg_addr_t *addrp ) { return ( addrp->hnkoff * SEGPERHNK ) + addrp->segoff; } -static inline intgen_t +static inline int64_t inomap_lastseg( intgen_t hnkoff ) { if ( hnkoff == inomap.lastseg.hnkoff ) @@ -1125,8 +1125,11 @@ cb_add_inogrp( void *arg1, intgen_t fsfd lastsegp->segoff = 0; if (lastsegp->hnkoff == inomap.hnkmaplen) { - intgen_t numsegs; - intgen_t oldsize; + int64_t numsegs; + int64_t oldsize; + + oldsize = inomap.hnkmaplen * SEGPERHNK + * sizeof(i2gseg_t); inomap.hnkmaplen++; inomap.hnkmap = (hnk_t *) @@ -1140,8 +1143,6 @@ cb_add_inogrp( void *arg1, intgen_t fsfd return -1; /* zero the new portion of the i2gmap */ - oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t); - memset(inomap.i2gmap + oldsize, 0, SEGPERHNK * sizeof(i2gseg_t)); @@ -1199,8 +1200,8 @@ static bool_t inomap_find_hnk( seg_addr_t *addrp, xfs_ino_t ino ) { hnk_t *hunkp; - intgen_t lower; - intgen_t upper; + int64_t lower; + int64_t upper; lower = 0; upper = inomap.lastseg.hnkoff; @@ -1231,8 +1232,8 @@ static bool_t inomap_find_seg( seg_addr_t *addrp, xfs_ino_t ino ) { seg_t *segp; - intgen_t lower; - intgen_t upper; + int64_t lower; + int64_t upper; if ( !inomap_validaddr( addrp ) ) { inomap_reset_context( addrp ); _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xfsdump: use 64bit local variables in inode.c 2015-08-21 14:01 ` [PATCH 1/2] xfsdump: use 64bit local variables in inode.c rjohnston @ 2015-08-21 17:03 ` Eric Sandeen 2015-08-21 18:22 ` Eric Sandeen 1 sibling, 0 replies; 13+ messages in thread From: Eric Sandeen @ 2015-08-21 17:03 UTC (permalink / raw) To: rjohnston, xfs On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: > The memset in cb_add_inogrp will segfault when the index oldsize > overflows. In cb_add_inogrp(), the temp variables used in > calculating the new i2gmap segment offset should be int64 instead > of intgen_t (int32). > > Fix this: > a. simplify the calculation of oldsize by moving it > before hnkmaplen is incremented. I think it'd make more sense to put a) in the second patch; it's unrelated to the variable size changes. Still wrapping my head around what these variables are for, but makes sense at first glance. Thanks, -Eric > b. change the index variables int64 to prevent overflow. > > Signed-off-by: Rich Johnston <rjohnston@sgi.com> > --- > dump/inomap.c | 41 +++++++++++++++++++++-------------------- > 1 file changed, 21 insertions(+), 20 deletions(-) > > Index: b/dump/inomap.c > =================================================================== > --- a/dump/inomap.c > +++ b/dump/inomap.c > @@ -71,7 +71,7 @@ static intgen_t cb_context( bool_t last, > drange_t *, > startpt_t *, > size_t, > - intgen_t, > + int64_t, > bool_t, > bool_t *); > static void cb_context_free( void ); > @@ -96,7 +96,7 @@ static off64_t estimate_dump_space( xfs_ > > /* inomap primitives > */ > -static intgen_t inomap_init( intgen_t igrpcnt ); > +static intgen_t inomap_init( int64_t igrpcnt ); > static void inomap_add( void *, xfs_ino_t ino, gen_t gen, intgen_t ); > static intgen_t inomap_set_state( void *, xfs_ino_t ino, intgen_t ); > static void inomap_set_gen(void *, xfs_ino_t, gen_t ); > @@ -160,7 +160,7 @@ inomap_build( jdm_fshandle_t *fshandlep, > xfs_bstat_t *bstatbufp; > size_t bstatbuflen; > bool_t pruneneeded = BOOL_FALSE; > - intgen_t igrpcnt = 0; > + int64_t igrpcnt = 0; > intgen_t stat; > intgen_t rval; > > @@ -449,7 +449,7 @@ cb_context( bool_t last, > drange_t *resumerangep, > startpt_t *startptp, > size_t startptcnt, > - intgen_t igrpcnt, > + int64_t igrpcnt, > bool_t skip_unchanged_dirs, > bool_t *pruneneededp ) > { > @@ -949,14 +949,14 @@ struct i2gseg { > typedef struct i2gseg i2gseg_t; > > typedef struct seg_addr { > - intgen_t hnkoff; > - intgen_t segoff; > - intgen_t inooff; > + int64_t hnkoff; > + int64_t segoff; > + int64_t inooff; > } seg_addr_t; > > static struct inomap { > hnk_t *hnkmap; > - intgen_t hnkmaplen; > + int64_t hnkmaplen; > i2gseg_t *i2gmap; > seg_addr_t lastseg; > } inomap; > @@ -1040,7 +1040,7 @@ SEG_GET_BITS( seg_t *segp, xfs_ino_t ino > /* context for inomap construction - initialized by map_init > */ > static intgen_t > -inomap_init( intgen_t igrpcnt ) > +inomap_init( int64_t igrpcnt ) > { > ASSERT( sizeof( hnk_t ) == HNKSZ ); > > @@ -1066,7 +1066,7 @@ inomap_getsz( void ) > static inline bool_t > inomap_validaddr( seg_addr_t *addrp ) > { > - intgen_t maxseg; > + int64_t maxseg; > > if ( addrp->hnkoff < 0 || addrp->hnkoff > inomap.lastseg.hnkoff ) > return BOOL_FALSE; > @@ -1093,13 +1093,13 @@ inomap_addr2seg( seg_addr_t *addrp ) > return &hunkp->seg[addrp->segoff]; > } > > -static inline intgen_t > +static inline int64_t > inomap_addr2segix( seg_addr_t *addrp ) > { > return ( addrp->hnkoff * SEGPERHNK ) + addrp->segoff; > } > > -static inline intgen_t > +static inline int64_t > inomap_lastseg( intgen_t hnkoff ) > { > if ( hnkoff == inomap.lastseg.hnkoff ) > @@ -1125,8 +1125,11 @@ cb_add_inogrp( void *arg1, intgen_t fsfd > lastsegp->segoff = 0; > > if (lastsegp->hnkoff == inomap.hnkmaplen) { > - intgen_t numsegs; > - intgen_t oldsize; > + int64_t numsegs; > + int64_t oldsize; > + > + oldsize = inomap.hnkmaplen * SEGPERHNK > + * sizeof(i2gseg_t); > > inomap.hnkmaplen++; > inomap.hnkmap = (hnk_t *) > @@ -1140,8 +1143,6 @@ cb_add_inogrp( void *arg1, intgen_t fsfd > return -1; > > /* zero the new portion of the i2gmap */ > - oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t); > - > memset(inomap.i2gmap + oldsize, > 0, > SEGPERHNK * sizeof(i2gseg_t)); > @@ -1199,8 +1200,8 @@ static bool_t > inomap_find_hnk( seg_addr_t *addrp, xfs_ino_t ino ) > { > hnk_t *hunkp; > - intgen_t lower; > - intgen_t upper; > + int64_t lower; > + int64_t upper; > > lower = 0; > upper = inomap.lastseg.hnkoff; > @@ -1231,8 +1232,8 @@ static bool_t > inomap_find_seg( seg_addr_t *addrp, xfs_ino_t ino ) > { > seg_t *segp; > - intgen_t lower; > - intgen_t upper; > + int64_t lower; > + int64_t upper; > > if ( !inomap_validaddr( addrp ) ) { > inomap_reset_context( addrp ); > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] xfsdump: use 64bit local variables in inode.c 2015-08-21 14:01 ` [PATCH 1/2] xfsdump: use 64bit local variables in inode.c rjohnston 2015-08-21 17:03 ` Eric Sandeen @ 2015-08-21 18:22 ` Eric Sandeen 1 sibling, 0 replies; 13+ messages in thread From: Eric Sandeen @ 2015-08-21 18:22 UTC (permalink / raw) To: rjohnston, xfs On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: > The memset in cb_add_inogrp will segfault when the index oldsize > overflows. In cb_add_inogrp(), the temp variables used in > calculating the new i2gmap segment offset should be int64 instead > of intgen_t (int32). > > Fix this: > a. simplify the calculation of oldsize by moving it > before hnkmaplen is incremented. > b. change the index variables int64 to prevent overflow. Ok, so looking at how we count inode groups, it's down this path: inomap_build( ... igrpcnt ...) inogrp_iter( fsfd, cb_count_inogrp, (void *)&igrpcnt, &stat ); cb_context( ... igrpcnt ... ) inomap_init(igrpcnt) We fill in igrpcnt with the inogrp_iter() call, which does bulkstats in groups / sizes of INOGRPLEN, which is: #define INOGRPLEN 256 and for each "group" of 256 inodes, igrpcnt increments by one, right? so I guess that means that if we have more than 2^31 inode groups, i.e. 2^31 * 256 = 500 billion inodes, (int) igrpcnt could overflow. That's probably not what you're trying to solve, right? So does igrpcnt really need to be 64 bits? If this is just about overflowing oldsize because it's in bytes, maybe my suggestion for patch number 2 solves that without the need for a bunch more 64 bit vars - in fact it getse rid of "oldsize" altogether. But more questions below... (Sorry, I don't really grok xfsdump, just trying to follow things through by rote, so bear with me ...) > Signed-off-by: Rich Johnston <rjohnston@sgi.com> > --- > dump/inomap.c | 41 +++++++++++++++++++++-------------------- > 1 file changed, 21 insertions(+), 20 deletions(-) > > Index: b/dump/inomap.c > =================================================================== > --- a/dump/inomap.c > +++ b/dump/inomap.c > @@ -71,7 +71,7 @@ static intgen_t cb_context( bool_t last, > drange_t *, > startpt_t *, > size_t, > - intgen_t, > + int64_t, > bool_t, > bool_t *); > static void cb_context_free( void ); > @@ -96,7 +96,7 @@ static off64_t estimate_dump_space( xfs_ > > /* inomap primitives > */ > -static intgen_t inomap_init( intgen_t igrpcnt ); > +static intgen_t inomap_init( int64_t igrpcnt ); > static void inomap_add( void *, xfs_ino_t ino, gen_t gen, intgen_t ); > static intgen_t inomap_set_state( void *, xfs_ino_t ino, intgen_t ); > static void inomap_set_gen(void *, xfs_ino_t, gen_t ); > @@ -160,7 +160,7 @@ inomap_build( jdm_fshandle_t *fshandlep, > xfs_bstat_t *bstatbufp; > size_t bstatbuflen; > bool_t pruneneeded = BOOL_FALSE; > - intgen_t igrpcnt = 0; > + int64_t igrpcnt = 0; > intgen_t stat; > intgen_t rval; > > @@ -449,7 +449,7 @@ cb_context( bool_t last, > drange_t *resumerangep, > startpt_t *startptp, > size_t startptcnt, > - intgen_t igrpcnt, > + int64_t igrpcnt, > bool_t skip_unchanged_dirs, > bool_t *pruneneededp ) > { > @@ -949,14 +949,14 @@ struct i2gseg { > typedef struct i2gseg i2gseg_t; > > typedef struct seg_addr { > - intgen_t hnkoff; > - intgen_t segoff; > - intgen_t inooff; > + int64_t hnkoff; > + int64_t segoff; > + int64_t inooff; What are these offsets "into?" Can these offset values really exceed 32 bits? I'll have to admit I'm lost, but I don't see it yet in the code. OTOH, if they can, then things like static inline intgen_t inomap_lastseg( intgen_t hnkoff ) { if ( hnkoff == inomap.lastseg.hnkoff ) return inomap.lastseg.segoff; else return SEGPERHNK - 1; } would need these type changes chased through as well, right? > } seg_addr_t; > > static struct inomap { > hnk_t *hnkmap; > - intgen_t hnkmaplen; > + int64_t hnkmaplen; hnkmaplen is initialized in inomap_init as: inomap.hnkmaplen = (igrpcnt + SEGPERHNK - 1) / SEGPERHNK; so if 32 bits is sufficient for igrpcnt, it's sufficient for hnkmaplen too. (after init, it only increments one at a time). > i2gseg_t *i2gmap; > seg_addr_t lastseg; > } inomap; > @@ -1040,7 +1040,7 @@ SEG_GET_BITS( seg_t *segp, xfs_ino_t ino > /* context for inomap construction - initialized by map_init > */ > static intgen_t > -inomap_init( intgen_t igrpcnt ) > +inomap_init( int64_t igrpcnt ) > { > ASSERT( sizeof( hnk_t ) == HNKSZ ); > > @@ -1066,7 +1066,7 @@ inomap_getsz( void ) > static inline bool_t > inomap_validaddr( seg_addr_t *addrp ) > { > - intgen_t maxseg; > + int64_t maxseg; ... maxseg = ( addrp->hnkoff == inomap.lastseg.hnkoff ) ? inomap.lastseg.segoff : SEGPERHNK - 1; so maxseg just needs to be the same type as segoff.... > > if ( addrp->hnkoff < 0 || addrp->hnkoff > inomap.lastseg.hnkoff ) > return BOOL_FALSE; > @@ -1093,13 +1093,13 @@ inomap_addr2seg( seg_addr_t *addrp ) > return &hunkp->seg[addrp->segoff]; > } > > -static inline intgen_t > +static inline int64_t > inomap_addr2segix( seg_addr_t *addrp ) > { > return ( addrp->hnkoff * SEGPERHNK ) + addrp->segoff; > } > > -static inline intgen_t > +static inline int64_t > inomap_lastseg( intgen_t hnkoff ) > { > if ( hnkoff == inomap.lastseg.hnkoff ) > @@ -1125,8 +1125,11 @@ cb_add_inogrp( void *arg1, intgen_t fsfd > lastsegp->segoff = 0; > > if (lastsegp->hnkoff == inomap.hnkmaplen) { > - intgen_t numsegs; > - intgen_t oldsize; > + int64_t numsegs; > + int64_t oldsize; > + > + oldsize = inomap.hnkmaplen * SEGPERHNK > + * sizeof(i2gseg_t); > > inomap.hnkmaplen++; > inomap.hnkmap = (hnk_t *) > @@ -1140,8 +1143,6 @@ cb_add_inogrp( void *arg1, intgen_t fsfd > return -1; > > /* zero the new portion of the i2gmap */ > - oldsize = (numsegs - SEGPERHNK) * sizeof(i2gseg_t); > - > memset(inomap.i2gmap + oldsize, > 0, > SEGPERHNK * sizeof(i2gseg_t)); > @@ -1199,8 +1200,8 @@ static bool_t > inomap_find_hnk( seg_addr_t *addrp, xfs_ino_t ino ) > { > hnk_t *hunkp; > - intgen_t lower; > - intgen_t upper; > + int64_t lower; > + int64_t upper; > > lower = 0; > upper = inomap.lastseg.hnkoff; > @@ -1231,8 +1232,8 @@ static bool_t > inomap_find_seg( seg_addr_t *addrp, xfs_ino_t ino ) > { > seg_t *segp; > - intgen_t lower; > - intgen_t upper; > + int64_t lower; > + int64_t upper; > > if ( !inomap_validaddr( addrp ) ) { > inomap_reset_context( addrp ); > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] xfsdump: don't do pointer math twice 2015-08-21 14:01 [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp rjohnston 2015-08-21 14:01 ` [PATCH 1/2] xfsdump: use 64bit local variables in inode.c rjohnston @ 2015-08-21 14:01 ` rjohnston 2015-08-21 17:24 ` Eric Sandeen 2015-08-21 15:47 ` [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp Eric Sandeen 2 siblings, 1 reply; 13+ messages in thread From: rjohnston @ 2015-08-21 14:01 UTC (permalink / raw) To: xfs [-- Attachment #1: fix-pointer-math-in-cb_add_inogrp.patch --] [-- Type: text/plain, Size: 1108 bytes --] The pointer math when calculating the address for the call to memset is incorrect, so we are clearing the wrong memory location. i2gmap is of type i2gseg_t oldsize has already computed the pointer offset oldsize = inomap.hnkmaplen * SEGPERHNK * sizeof(i2gseg_t); the memset call is using inomap.i2gmap + oldsize == &inomap.i2gmap[oldsize] so we were doing the pointer math twice. We already compensate for the length of each item in oldsize so adding need to add a (char *) cast to the memset parameter. --- dump/inomap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: b/dump/inomap.c =================================================================== --- a/dump/inomap.c +++ b/dump/inomap.c @@ -1143,7 +1143,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd return -1; /* zero the new portion of the i2gmap */ - memset(inomap.i2gmap + oldsize, + memset((char *)inomap.i2gmap + oldsize, 0, SEGPERHNK * sizeof(i2gseg_t)); } _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] xfsdump: don't do pointer math twice 2015-08-21 14:01 ` [PATCH 2/2] xfsdump: don't do pointer math twice rjohnston @ 2015-08-21 17:24 ` Eric Sandeen 0 siblings, 0 replies; 13+ messages in thread From: Eric Sandeen @ 2015-08-21 17:24 UTC (permalink / raw) To: rjohnston, xfs On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: > The pointer math when calculating the address for the call to memset > is incorrect, so we are clearing the wrong memory location. > > i2gmap is of type i2gseg_t > > oldsize has already computed the pointer offset > oldsize = inomap.hnkmaplen * SEGPERHNK * sizeof(i2gseg_t); > the memset call is using > inomap.i2gmap + oldsize == &inomap.i2gmap[oldsize] > so we were doing the pointer math twice. > > We already compensate for the length of each item in oldsize so > adding need to add a (char *) cast to the memset parameter. What about just doing: memset(&inomap.i2gmap[numsegs - SEGPERHNK], 0, SEGPERHNK * sizeof(i2gseg_t)); which more clearly shows that we're setting the new array members to zero. (could do oldsegs = inomap.hnkmaplen * SEGPERHNK; prior to the hnkmaplen++, if that makes it any more readable...) *shrug* it seems a little clearer to me, anyway. Thanks, -Eric > --- > dump/inomap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: b/dump/inomap.c > =================================================================== > --- a/dump/inomap.c > +++ b/dump/inomap.c > @@ -1143,7 +1143,7 @@ cb_add_inogrp( void *arg1, intgen_t fsfd > return -1; > > /* zero the new portion of the i2gmap */ > - memset(inomap.i2gmap + oldsize, > + memset((char *)inomap.i2gmap + oldsize, > 0, > SEGPERHNK * sizeof(i2gseg_t)); > } > > _______________________________________________ > xfs mailing list > xfs@oss.sgi.com > http://oss.sgi.com/mailman/listinfo/xfs > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 14:01 [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp rjohnston 2015-08-21 14:01 ` [PATCH 1/2] xfsdump: use 64bit local variables in inode.c rjohnston 2015-08-21 14:01 ` [PATCH 2/2] xfsdump: don't do pointer math twice rjohnston @ 2015-08-21 15:47 ` Eric Sandeen 2015-08-21 16:38 ` Rich Johnston 2015-08-21 20:08 ` Rich Johnston 2 siblings, 2 replies; 13+ messages in thread From: Eric Sandeen @ 2015-08-21 15:47 UTC (permalink / raw) To: rjohnston, xfs On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: > The memset in cb_add_inogrp will segfault when the index oldsize > overflows. In cb_add_inogrp(), the temp variables used in > calculating the new i2gmap segment offset should be int64 instead > of intgen_t (int32). > > A second bug also occurs because we already compensate for the > length of each item in oldsize so are 32bit wrap becomes a 40bit > wrap. Hi - Are there any testcases for these? xfsdump is alien code, I swear; I'm not quite sure offhand how to tickle any of these bugs. Thanks, -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 15:47 ` [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp Eric Sandeen @ 2015-08-21 16:38 ` Rich Johnston 2015-08-21 16:39 ` Eric Sandeen 2015-08-21 20:08 ` Rich Johnston 1 sibling, 1 reply; 13+ messages in thread From: Rich Johnston @ 2015-08-21 16:38 UTC (permalink / raw) To: Eric Sandeen, xfs On 08/21/2015 10:47 AM, Eric Sandeen wrote: > On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: >> The memset in cb_add_inogrp will segfault when the index oldsize >> overflows. In cb_add_inogrp(), the temp variables used in >> calculating the new i2gmap segment offset should be int64 instead >> of intgen_t (int32). >> >> A second bug also occurs because we already compensate for the >> length of each item in oldsize so are 32bit wrap becomes a 40bit >> wrap. > > Hi - > > Are there any testcases for these? xfsdump is alien code, I swear; > I'm not quite sure offhand how to tickle any of these bugs. > > Thanks, > -Eric > No I thought simple examination shows the bug. It was a customer bug. The number of inodes that we needed before wrapping was a couple hundred inodes. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 16:38 ` Rich Johnston @ 2015-08-21 16:39 ` Eric Sandeen 2015-08-21 16:49 ` Rich Johnston 0 siblings, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2015-08-21 16:39 UTC (permalink / raw) To: Rich Johnston, xfs On 8/21/15 11:38 AM, Rich Johnston wrote: > On 08/21/2015 10:47 AM, Eric Sandeen wrote: >> On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: >>> The memset in cb_add_inogrp will segfault when the index oldsize >>> overflows. In cb_add_inogrp(), the temp variables used in >>> calculating the new i2gmap segment offset should be int64 instead >>> of intgen_t (int32). >>> >>> A second bug also occurs because we already compensate for the >>> length of each item in oldsize so are 32bit wrap becomes a 40bit >>> wrap. >> >> Hi - >> >> Are there any testcases for these? xfsdump is alien code, I swear; >> I'm not quite sure offhand how to tickle any of these bugs. >> >> Thanks, >> -Eric >> > No I thought simple examination shows the bug. Nothing is simple in xfsdump, IMHO. At least to the uninitiated. :) > It was a customer bug. > > The number of inodes that we needed before wrapping was a couple hundred inodes. I did eventually manage to hit the segfault, thanks. -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 16:39 ` Eric Sandeen @ 2015-08-21 16:49 ` Rich Johnston 0 siblings, 0 replies; 13+ messages in thread From: Rich Johnston @ 2015-08-21 16:49 UTC (permalink / raw) To: Eric Sandeen, xfs On 08/21/2015 11:39 AM, Eric Sandeen wrote: > On 8/21/15 11:38 AM, Rich Johnston wrote: >> On 08/21/2015 10:47 AM, Eric Sandeen wrote: >>> On 8/21/15 9:01 AM, rjohnston@sgi.com wrote: >>>> The memset in cb_add_inogrp will segfault when the index oldsize >>>> overflows. In cb_add_inogrp(), the temp variables used in >>>> calculating the new i2gmap segment offset should be int64 instead >>>> of intgen_t (int32). >>>> >>>> A second bug also occurs because we already compensate for the >>>> length of each item in oldsize so are 32bit wrap becomes a 40bit >>>> wrap. >>> >>> Hi - >>> >>> Are there any testcases for these? xfsdump is alien code, I swear; >>> I'm not quite sure offhand how to tickle any of these bugs. >>> >>> Thanks, >>> -Eric >>> >> No I thought simple examination shows the bug. > > Nothing is simple in xfsdump, IMHO. At least to the uninitiated. :) > :) >> It was a customer bug. >> >> The number of inodes that we needed before wrapping was a couple hundred inodes. ^^^^ make that *million* > > I did eventually manage to hit the segfault, thanks. > > -Eric > _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 15:47 ` [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp Eric Sandeen 2015-08-21 16:38 ` Rich Johnston @ 2015-08-21 20:08 ` Rich Johnston 2015-08-21 20:11 ` Eric Sandeen 1 sibling, 1 reply; 13+ messages in thread From: Rich Johnston @ 2015-08-21 20:08 UTC (permalink / raw) To: Eric Sandeen, xfs On 08/21/2015 10:47 AM, Eric Sandeen wrote: > Hi - > > Are there any testcases for these? xfsdump is alien code, I swear; > I'm not quite sure offhand how to tickle any of these bugs. > > Thanks, > -Eric > Hey Eric, Many thanks for taking the time to review these patches. I take a look at all your responses and get a V2 out early next week. Thanks --Rich _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 20:08 ` Rich Johnston @ 2015-08-21 20:11 ` Eric Sandeen 2015-08-26 14:33 ` Rich Johnston 0 siblings, 1 reply; 13+ messages in thread From: Eric Sandeen @ 2015-08-21 20:11 UTC (permalink / raw) To: Rich Johnston, xfs On 8/21/15 3:08 PM, Rich Johnston wrote: > On 08/21/2015 10:47 AM, Eric Sandeen wrote: >> Hi - >> >> Are there any testcases for these? xfsdump is alien code, I swear; >> I'm not quite sure offhand how to tickle any of these bugs. >> >> Thanks, >> -Eric >> > > Hey Eric, > > Many thanks for taking the time to review these patches. > I take a look at all your responses and get a V2 out early next week. Sounds good. If I was off track about the need for the extra 64-bit vars, just educate me. ;) -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp 2015-08-21 20:11 ` Eric Sandeen @ 2015-08-26 14:33 ` Rich Johnston 0 siblings, 0 replies; 13+ messages in thread From: Rich Johnston @ 2015-08-26 14:33 UTC (permalink / raw) To: Eric Sandeen, xfs On 08/21/2015 03:11 PM, Eric Sandeen wrote: > On 8/21/15 3:08 PM, Rich Johnston wrote: >> On 08/21/2015 10:47 AM, Eric Sandeen wrote: >>> Hi - >>> >>> Are there any testcases for these? xfsdump is alien code, I swear; >>> I'm not quite sure offhand how to tickle any of these bugs. >>> >>> Thanks, >>> -Eric >>> >> >> Hey Eric, >> >> Many thanks for taking the time to review these patches. >> I take a look at all your responses and get a V2 out early next week. > > Sounds good. If I was off track about the need for the extra 64-bit vars, > just educate me. ;) > > -Eric > Ok you convinced me, there is no need for patch 1 and patch 2 can be solved by removing the local variable oldsize. A new patch has been submitted. [PATCH] xfsdump: prevent segfault in cb_add_inogrp _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-08-26 14:33 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-08-21 14:01 [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp rjohnston 2015-08-21 14:01 ` [PATCH 1/2] xfsdump: use 64bit local variables in inode.c rjohnston 2015-08-21 17:03 ` Eric Sandeen 2015-08-21 18:22 ` Eric Sandeen 2015-08-21 14:01 ` [PATCH 2/2] xfsdump: don't do pointer math twice rjohnston 2015-08-21 17:24 ` Eric Sandeen 2015-08-21 15:47 ` [PATCH 0/2] xfsdump: fix problems in cb_add_inogrp Eric Sandeen 2015-08-21 16:38 ` Rich Johnston 2015-08-21 16:39 ` Eric Sandeen 2015-08-21 16:49 ` Rich Johnston 2015-08-21 20:08 ` Rich Johnston 2015-08-21 20:11 ` Eric Sandeen 2015-08-26 14:33 ` Rich Johnston
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox