Random xfsdump fixes
cover Random xfsdump fixes
This is the result of needing some mindless work and going through coverity errors.
It knocks out about 30 issues.
Tested w/ xfstests, -g dump.
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/9 xfsdump: don't try to close fd from failed open
I was going to just return if open() fails, but realized that failing ioctls on the not-opened fd are ... maybe intentional? By trying to run the ioctls on the invalid fd, it actually spits useful error messages to the log, so...
--- restore/tree.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
restore/tree.c +2 -1
@@ -2626,7 +2626,8 @@ setdirattr( dah_t dah, char *path ) path, strerror(errno)); } - (void )close ( fd ); +if (fd >=0 ) +close ( fd ); }/* deletes orphanage if empty, else warns
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/9 xfsdump: Fix overflow of "question" string in Media_prompt_erase()
In reply to Eric SandeenThe string we were sprintf'ing is trivially longer than the array we allocated for it.
Allocate something big enough, and snprintf to avoid overflow
--- dump/content.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
dump/content.c +2 -2
@@ -6174,7 +6174,7 @@ static bool_t Media_prompt_overwrite( drive_t *drivep ) {fold_t fold; -char question[100 ]; +char question[110 ];char *preamblestr[PREAMBLEMAX ];size_t preamblecnt;char *querystr[QUERYMAX ];
@@ -6201,7 +6201,7 @@ retry:/* query: ask if overwrite ok */ -sprintf ( question, +snprintf ( question,sizeof (question),"overwrite data on media in " "drive %u?\n" , (unsigned int )drivep->d_index );
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/9] xfsdump: Fix overflow of "question" string in Media_prompt_erase()
Quoted 4 lines · Eric Sandeen
The string we were sprintf'ing is trivially longer than the array we allocated for it.
Allocate something big enough, and snprintf to avoid overflow
How about switching it to asprintf() to get rid of that dangerous static buffer entirely?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH v2 2/9 xfsdump: Fix overflow of "question" string in Media_prompt_erase()
In reply to Eric SandeenThe string we were sprintf'ing is trivially longer than the array we allocated for it.
Use asprintf instead, as suggested by Christoph.
---
V2: use asprintf
dump/content.c +5 -4
@@ -6174,7 +6174,7 @@ static bool_t Media_prompt_overwrite( drive_t *drivep ) {fold_t fold; -char question[100 ]; +char *question;char *preamblestr[PREAMBLEMAX ];size_t preamblecnt;char *querystr[QUERYMAX ];
@@ -6201,9 +6201,8 @@ retry:/* query: ask if overwrite ok */ -sprintf ( question, -"overwrite data on media in " -"drive %u?\n" , +asprintf ( &question, +"overwrite data on media in drive %u?\n" , (unsigned int )drivep->d_index ); querycnt =0 ; querystr[ querycnt++ ] = question;
@@ -6250,6 +6249,8 @@ retry:dlog_end ( postamblestr, postamblecnt ); +free (question); +if ( responseix == sigintix ) {if (cldmgr_stop_requested ( )) {return BOOL_FALSE ;
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/9 V2] xfsdump: Fix overflow of "question" string in Media_prompt_erase()
dump/content.c retry: · line 6206
Quoted 3 lines
dump/content.c +2
-0+asprintf ( &question, +"overwrite data on media in drive %u?\n" , (unsigned int )drivep->d_index );
Where is the error handling?
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/9 V2] xfsdump: Fix overflow of "question" string in Media_prompt_erase()
Quoted 5 lines · Christoph Hellwig
dump/content.c retry: · line 6206
Quoted 3 lines
dump/content.c +2
-0+asprintf ( &question, +"overwrite data on media in drive %u?\n" , (unsigned int )drivep->d_index );Where is the error handling?
Well, ok.
The error handling ahead of this is crap; if the
function returns !ok, we change the media:
ok = Media_prompt_overwrite( drivep );
if ( intr_allowed && cldmgr_stop_requested( )) {
return RV_INTR;
}
if ( ! ok ) {
goto changemedia;
}I don't want to try to understand and rework all the ways we can back out of this if we don't have ~100 bytes available.
So, what would you prefer:
I could make a 110-byte array, and snprintf max 110 bytes to it. Or I could asprintf, and exit the whole program if it fails.
I'm not trying to make xfsdump perfect, I'm trying to make it suck less. ;)
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/9 xfsdump: fix wrong test for hard stack limit change failure
In reply to Eric SandeenWe test if ( rlimit64.rlim_cur < minstacksz ) to see whether we failed to increase rlimit64.rlim_max? Weird. Fix it.
(Granted, we raise both cur and max together, but it's odd to test one, and report the other).
--- common/main.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
common/main.c +1 -1
@@ -2077,7 +2077,7 @@ set_rlimits( size64_t *vmszp ) (void )setrlimit64 (RLIMIT_STACK , &rlimit64 ); rval =getrlimit64 (RLIMIT_STACK , &rlimit64 );ASSERT ( ! rval ); -if ( rlimit64.rlim_cur < minstacksz ) { +if ( rlimit64.rlim_max < minstacksz ) {mlog (MLOG_NORMAL | MLOG_WARNING
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/9 xfsdump: remove pointless if (dirty);
In reply to Eric Sandeen"dirty" seems to be a remnant of unfinished code; other bits of it are there but commented out, so comment out "if (dirty);" as well, which is a no-op if statement which can't be true.
--- inventory/inv_stobj.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
inventory/inv_stobj.c +2 -0
@@ -976,7 +976,9 @@ stobj_delete_mobj(int fd, }free ( mfiles ); +/* if ( dirty ); +*/ }free ( strms );
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/9] xfsdump: remove pointless if (dirty);
Quoted 14 lines · Eric Sandeen
"dirty" seems to be a remnant of unfinished code; other bits of it are there but commented out, so comment out "if (dirty);" as well, which is a no-op if statement which can't be true.
Signed-off-by Eric Sandeen<[email protected]>--- inventory/inv_stobj.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)inventory/inv_stobj.c +0
-0
inventory/inv_stobj.c stobj_delete_mobj(int fd, · line 984
Quoted 9 lines
inventory/inv_stobj.c +2
-0@@ -976,7 +976,9 @@ stobj_delete_mobj(int fd, }free ( mfiles ); +/* if ( dirty ); +*/ }free ( strms );
Why not remove all the references to dirty in the function? Like you said, it doesn't do anything.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/9] xfsdump: remove pointless if (dirty);
Quoted 23 lines · Mark Tinguely
Quoted 22 lines · Eric Sandeen
"dirty" seems to be a remnant of unfinished code; other bits of it are there but commented out, so comment out "if (dirty);" as well, which is a no-op if statement which can't be true.
Signed-off-by Eric Sandeen<[email protected]>--- inventory/inv_stobj.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)inventory/inv_stobj.c +2
-0@@ -976,7 +976,9 @@ stobj_delete_mobj(int fd, }free ( mfiles ); +/* if ( dirty ); +*/ }
Quoted 4 lines
inventory/inv_stobj.c stobj_delete_mobj(int fd, · line 984
Quoted 1 line
inventory/inv_stobj.c +0
-0free ( strms );Why not remove all the references to dirty in the function? Like you said, it doesn't do anything.
Somebody left this and the rest of the commented-out code there for some documentation reason, it seems, so I figured...
It's not like the rest of the code is a thing of beauty, and this wart is ruining its perfection. ;)
I have no idea, honestly. xfsdump is dropped in from some alien world. Somebody @ SGI might know, or could find out by looking at long-lost history?
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/9] xfsdump: remove pointless if (dirty);
Quoted 32 lines · Eric Sandeen
Quoted 23 lines · Mark Tinguely
Quoted 22 lines · Eric Sandeen
"dirty" seems to be a remnant of unfinished code; other bits of it are there but commented out, so comment out "if (dirty);" as well, which is a no-op if statement which can't be true.
Signed-off-by Eric Sandeen<[email protected]>--- inventory/inv_stobj.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)inventory/inv_stobj.c +2
-0@@ -976,7 +976,9 @@ stobj_delete_mobj(int fd, }free ( mfiles ); +/* if ( dirty ); +*/ }Quoted 4 lines
inventory/inv_stobj.c stobj_delete_mobj(int fd, · line 984
Quoted 1 line
inventory/inv_stobj.c +0
-0free ( strms );Why not remove all the references to dirty in the function? Like you said, it doesn't do anything.
Somebody left this and the rest of the commented-out code there for some documentation reason, it seems, so I figured...
Quoted 8 lines
It's not like the rest of the code is a thing of beauty, and this wart is ruining its perfection. ;)
I have no idea, honestly. xfsdump is dropped in from some alien world. Somebody @ SGI might know, or could find out by looking at long-lost history?
-Eric
I will see what I can dig up.
We need to start pulling off the warts.
The interactive command line parsing is also damaged, I will find the patch and post it.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/9 xfsdump: remove some unreachable code
In reply to Eric Sandeenin create_windows() we assign INFO_SIZE (which is defined to 4) to the infosize variable, then test whether it is <= 0. Remove the pointless test.
in tsi_cmd_ls(), inside a if ( ! isdirpr ) block, we test whether isdirpr might be true... remove it.
--- invutil/cmenu.c | 2 -- restore/tree.c | 5 ++--- 2 files changed, 2 insertions(+), 5 deletions(-)
invutil/cmenu.c +0 -2
@@ -555,8 +555,6 @@ create_windows() menusize, COLS - 1); infosize =INFO_SIZE ; -if (infosize <=0 ) - infosize =1 ; infowin =newwin (infosize,COLS , menusize +1 ,0 );keypad (infowin, TRUE);
restore/tree.c +2 -3
@@ -2905,11 +2905,10 @@ tsi_cmd_ls( void *ctxp,*/ if ( ! isdirpr ) { ( * pcb )( pctxp, -" %s %10llu %s%s\n" , +" %s %10llu %s\n" , isselpr ?"*" :" " , ino, - tranp->t_inter.i_name, - isdirpr ?"/" :" " ); + tranp->t_inter.i_name);return ; }
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/9 xfsdump: free suboptstrs if mlog_init1 fails
In reply to Eric Sandeensuboptstrs was never freed on error paths.
--- common/mlog.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-)
common/mlog.c +11 -8
@@ -149,6 +149,7 @@ mlog_init1( intgen_t argc, char *argv[ ] )ix_t soix;size_t vsymcnt;intgen_t c; +bool_t retval =BOOL_FALSE ;/* Until proven otherwise */ /* prepare an array of suboption token strings. this will be the * concatenation of the subsystem names with the verbosity symbols.
@@ -192,7 +193,7 @@ mlog_init1( intgen_t argc, char *argv[ ] ) progname, c );usage ( ); -return BOOL_FALSE ; + goto out_free; } options = optarg;while ( *options ) {
@@ -208,7 +209,7 @@ mlog_init1( intgen_t argc, char *argv[ ] ) progname, c );usage ( ); -return BOOL_FALSE ; + goto out_free; } ASSERT( ( ix_t )suboptix <
@@ -224,7 +225,7 @@ mlog_init1( intgen_t argc, char *argv[ ] ) c, mlog_ss_names[ suboptix ] );usage ( ); -return BOOL_FALSE ; + goto out_free; } ssix = (ix_t )suboptix; mlog_level_ss[ ssix ] =
@@ -238,7 +239,7 @@ mlog_init1( intgen_t argc, char *argv[ ] ) progname, c );usage ( ); -return BOOL_FALSE ; + goto out_free; } ssix =MLOG_SS_GEN ; mlog_level_ss[ ssix ] =
@@ -251,7 +252,7 @@ mlog_init1( intgen_t argc, char *argv[ ] ) progname, c );usage ( ); -return BOOL_FALSE ; + goto out_free; } }break ;
@@ -267,8 +268,6 @@ mlog_init1( intgen_t argc, char *argv[ ] ) } } -free ( (void * )suboptstrs ); -/* give subsystems not explicitly called out the "general" verbosity */ for ( ssix =0 ; ssix <MLOG_SS_CNT ; ssix++ ) {
@@ -301,7 +300,11 @@ mlog_init1( intgen_t argc, char *argv[ ] ) mlog_qlockh =QLOCKH_NULL ; -return BOOL_TRUE ; + retval =BOOL_TRUE ; + +out_free: +free ( (void * )suboptstrs ); +return retval; } bool_t
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/9 xfsdump: free allocated persistent paths
In reply to Eric Sandeen"perspath" gets allocated by open_pathalloc, and haphazardly freed at best; almost never on errors, often not on success.
While we're in there, fix a few leaked fd's as well.
--- restore/content.c | 2 ++ restore/inomap.c | 11 +++++++++++ restore/tree.c | 9 +++++++++ 3 files changed, 22 insertions(+), 0 deletions(-)
restore/content.c +2 -0
@@ -1342,6 +1342,7 @@ content_init( intgen_t argc, char *argv[ ], size64_t vmsz ) "could not open/create persistent state file %s: %s\n"), perspath, strerror( errno )); +free (perspath);return BOOL_FALSE ; }
@@ -1356,6 +1357,7 @@ content_init( intgen_t argc, char *argv[ ], size64_t vmsz ) "could not map persistent state file hdr %s: %s\n"), perspath, strerror( errno )); +free (perspath);return BOOL_FALSE ; }
restore/inomap.c +11 -0
@@ -217,6 +217,7 @@ inomap_restore_pers( drive_t *drivep, "could not open %s: %s\n"), perspath, strerror( errno )); +free (perspath);return RV_ERROR ; }
@@ -233,6 +234,8 @@ inomap_restore_pers( drive_t *drivep, "unable to map %s: %s\n"), perspath, strerror( errno )); +close (fd); +free (perspath);return RV_ERROR ; }
@@ -363,6 +366,7 @@ inomap_sync_pers( char *hkdir ) perspath =open_pathalloc ( hkdir,PERS_NAME ,0 ); pers_fd =open ( perspath,O_RDWR );if ( pers_fd <0 ) { +free (perspath);return BOOL_TRUE ; }
@@ -377,6 +381,8 @@ inomap_sync_pers( char *hkdir ) "unable to map %s hdr: %s\n"), perspath, strerror( errno )); +close (pers_fd); +free (perspath);return BOOL_FALSE ; }
@@ -398,9 +404,14 @@ inomap_sync_pers( char *hkdir ) "unable to map %s: %s\n"), perspath, strerror( errno )); +close (pers_fd); +free (perspath);return BOOL_FALSE ; } +close (pers_fd); +free (perspath); +/* correct the next pointers */ for ( hnkp = roothnkp
restore/tree.c +9 -0
@@ -420,6 +420,7 @@ tree_init( char *hkdir, "could not open %s: %s\n"), perspath, strerror( errno )); +free (perspath);return BOOL_FALSE ; }
@@ -435,6 +436,7 @@ tree_init( char *hkdir, "unable to map %s: %s\n"), perspath, strerror( errno )); +free (perspath);return BOOL_FALSE ; }
@@ -443,9 +445,12 @@ tree_init( char *hkdir,*/ ok =hash_init ( vmsz /HASHSZ_PERVM , dircnt, nondircnt, perspath );if ( ! ok ) { +free (perspath);return BOOL_FALSE ; } +free (perspath); +/* initialize the node abstraction. let it's use of backing store * begin immediately after the hash abstraction. give it the remainder * of vm.
@@ -595,6 +600,7 @@ tree_sync( char *hkdir, "could not open %s: %s\n"), perspath, strerror( errno )); +free (perspath);return BOOL_FALSE ; }
@@ -610,6 +616,7 @@ tree_sync( char *hkdir, "unable to map %s: %s\n"), perspath, strerror( errno )); +free (perspath);return BOOL_FALSE ; }
@@ -632,9 +639,11 @@ tree_sync( char *hkdir,*/ ok =hash_sync ( perspath );if ( ! ok ) { +free (perspath);return BOOL_FALSE ; } +free (perspath);/* synchronize with the node abstraction. */ ASSERT ( persp->p_hashsz <= ( size64_t )(OFF64MAX - (off64_t )PERSSZ ));
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/9 xfsdump: children[] is an array of child pointers, not of child structures
In reply to Eric Sandeennode_t **children; /* child nodes */
so allocating space to hold nbr_children structures is wrong; we need to hold nbr_children _pointers_ to child structures.
It's over-allocating, so it's relatively harmless.
--- invutil/list.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
invutil/list.c +1 -1
@@ -34,7 +34,7 @@ parent_add_child(node_t *parent, node_t *child) d = parent->data; d->nbr_children++; - d->children =realloc (d->children, d->nbr_children *sizeof (*child)); + d->children =realloc (d->children, d->nbr_children *sizeof (node_t *)); d->children[d->nbr_children -1 ] = child;return parent;
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
PATCH 9/9 xfsdump: fix uninit ackstr in content_mediachange_query()
In reply to Eric SandeenToday, this sends an uninitialized ackstr[0] to be mlog'd - who knows what we get out of it. Other places follow this "count = 0, string = "\n"" pattern which seemsa bit odd, but better than printing uninitialized memory.
To be completely honest, I have no test for this.
--- restore/content.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
restore/content.c +1 -0
@@ -2915,6 +2915,7 @@ content_mediachange_query( void ) bagp =0 ; } ackcnt =0 ; + ackstr[ ackcnt++ ] ="\n" ;dlog_multi_ack ( ackstr, ackcnt ); querycnt =0 ;
Signature · 6 lines
1.7.1 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 9/9] xfsdump: fix uninit ackstr in content_mediachange_query()
Quoted 17 lines · Eric Sandeen
Today, this sends an uninitialized ackstr[0] to be mlog'd - who knows what we get out of it. Other places follow this "count = 0, string = "\n"" pattern which seemsa bit odd, but better than printing uninitialized memory.
To be completely honest, I have no test for this.
Signed-off-by Eric Sandeen<[email protected]>--- restore/content.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)restore/content.c +0
-0
restore/content.c content_mediachange_query( void ) · line 2921
Quoted 7 lines
restore/content.c +1
-0@@ -2915,6 +2915,7 @@ content_mediachange_query( void ) bagp =0 ; } ackcnt =0 ; + ackstr[ ackcnt++ ] ="\n" ;dlog_multi_ack ( ackstr, ackcnt ); querycnt =0 ;
dlog_multi_ack() with count of 0 will exit without doing anything.
Looks like some conditional code that filled the ackstr array (like the other callers) was removed. I vote to pull ackstr, ackcnt and the dlog_multi_ack() from this function.
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 9/9] xfsdump: fix uninit ackstr in content_mediachange_query()
Quoted 13 lines · Mark Tinguely
Quoted 12 lines · Eric Sandeen
Today, this sends an uninitialized ackstr[0] to be mlog'd - who knows what we get out of it. Other places follow this "count = 0, string = "\n"" pattern which seemsa bit odd, but better than printing uninitialized memory.
To be completely honest, I have no test for this.
Signed-off-by Eric Sandeen<[email protected]>--- restore/content.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Quoted 14 lines
restore/content.c content_mediachange_query( void ) · line 2921
Quoted 12 lines
restore/content.c +1
-0@@ -2915,6 +2915,7 @@ content_mediachange_query( void ) bagp =0 ; } ackcnt =0 ; + ackstr[ ackcnt++ ] ="\n" ;dlog_multi_ack ( ackstr, ackcnt ); querycnt =0 ;
dlog_multi_ack()with count of 0 will exit without doing anything.
Oh, yeah, I misread it. Sorry. I saw stuff like this in sigint_dialog()
ackcnt = 0;
ackstr[ ackcnt++ ] = "\n";
dlog_multi_ack( ackstr,
ackcnt );and got carried away.
Quoted 3 lines
Looks like some conditional code that filled the ackstr array (like the other callers) was removed. I vote to pull ackstr, ackcnt and the
dlog_multi_ack()from this function.
I wondered about that, but didn't want to go changing things I wasn't able to test or trivially understand.
However, I guess I'm challenged even in the latter this week. ;)
Anyway, yeah, just drop this patch, sorry.
Thanks for the review,
-Eric
Quoted 1 line
--Mark.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs