[PATCH 0/5] xfsprogs random fixes found by Coverity scan

[PATCH 0/5] xfsprogs random fixes found by Coverity scan

[Andrey Albershteyn]

Hi all,

This is bunch of random fixes found by Coverity scan, there's memory
leak, truncation of time_t to int, access overflow, and freeing of
uninitialized struct.

--
Andrey

Andrey Albershteyn (5):
  xfs_db: fix leak in flist_find_ftyp()
  xfs_repair: make duration take time_t
  xfs_io: init count to stop loop from execution
  xfs_scrub: don't call phase_end if phase_rusage was not initialized
  xfs_fsr: convert fsrallfs to use time_t instead of int

 db/flist.c          | 4 +++-
 fsr/xfs_fsr.c       | 4 ++--
 io/parent.c         | 2 +-
 repair/progress.c   | 4 ++--
 repair/progress.h   | 2 +-
 repair/xfs_repair.c | 2 +-
 scrub/xfs_scrub.c   | 3 ++-
 7 files changed, 12 insertions(+), 9 deletions(-)

-- 
2.42.0

[PATCH 1/5] xfs_db: fix leak in flist_find_ftyp()

[Andrey Albershteyn]

When count is zero fl reference is lost. Fix it by freeing the list.

Fixes: a0d79cb37a36 ("xfs_db: make flist_find_ftyp() to check for field existance on disk")
Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 db/flist.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/db/flist.c b/db/flist.c
index c81d229ab99c..0a6cc5fcee43 100644
--- a/db/flist.c
+++ b/db/flist.c
@@ -424,8 +424,10 @@ flist_find_ftyp(
 		if (f->ftyp == type)
 			return fl;
 		count = fcount(f, obj, startoff);
-		if (!count)
+		if (!count) {
+			flist_free(fl);
 			continue;
+		}
 		fa = &ftattrtab[f->ftyp];
 		if (fa->subfld) {
 			flist_t *nfl;
-- 
2.42.0

Re: [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp()

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 02:34:23PM +0200, Andrey Albershteyn wrote:
> When count is zero fl reference is lost. Fix it by freeing the list.
> 
> Fixes: a0d79cb37a36 ("xfs_db: make flist_find_ftyp() to check for field existance on disk")
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>

Yep, that's a leak.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  db/flist.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/db/flist.c b/db/flist.c
> index c81d229ab99c..0a6cc5fcee43 100644
> --- a/db/flist.c
> +++ b/db/flist.c
> @@ -424,8 +424,10 @@ flist_find_ftyp(
>  		if (f->ftyp == type)
>  			return fl;
>  		count = fcount(f, obj, startoff);
> -		if (!count)
> +		if (!count) {
> +			flist_free(fl);
>  			continue;
> +		}
>  		fa = &ftattrtab[f->ftyp];
>  		if (fa->subfld) {
>  			flist_t *nfl;
> -- 
> 2.42.0
> 
> 

Re: [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp()

[Christoph Hellwig]

On Tue, Apr 16, 2024 at 02:34:23PM +0200, Andrey Albershteyn wrote:
> +		if (!count) {
> +			flist_free(fl);
>  			continue;
> +		}

This looks good.  The more obvious way would be move the whole
loop body into a helper with two clear exits, one that returns
fl, and one that frees it and returns NULL..

[PATCH 2/5] xfs_repair: make duration take time_t

[Andrey Albershteyn]

In most of the uses of duration() takes time_t instead of int.
Convert the rest to use time_t and make duration() take time_t to
not truncate it to int.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 repair/progress.c   | 4 ++--
 repair/progress.h   | 2 +-
 repair/xfs_repair.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/repair/progress.c b/repair/progress.c
index f6c4d988444e..c2af1387eb14 100644
--- a/repair/progress.c
+++ b/repair/progress.c
@@ -273,7 +273,7 @@ progress_rpt_thread (void *p)
 	_("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"),
 				tmp->tm_hour, tmp->tm_min, tmp->tm_sec,
 				current_phase, percent,
-				duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));
+				duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));
 		}
 
 		if (pthread_mutex_unlock(&msgp->mutex) != 0) {
@@ -420,7 +420,7 @@ timestamp(int end, int phase, char *buf)
 }
 
 char *
-duration(int length, char *buf)
+duration(time_t length, char *buf)
 {
 	int sum;
 	int weeks;
diff --git a/repair/progress.h b/repair/progress.h
index 2c1690db1b17..9575df164aa0 100644
--- a/repair/progress.h
+++ b/repair/progress.h
@@ -38,7 +38,7 @@ extern void summary_report(void);
 extern int  set_progress_msg(int report, uint64_t total);
 extern uint64_t print_final_rpt(void);
 extern char *timestamp(int end, int phase, char *buf);
-extern char *duration(int val, char *buf);
+extern char *duration(time_t val, char *buf);
 extern int do_parallel;
 
 #define	PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b)
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index ba9d28330d82..78a7205f0054 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -377,7 +377,7 @@ process_args(int argc, char **argv)
 			do_prefetch = 0;
 			break;
 		case 't':
-			report_interval = (int)strtol(optarg, NULL, 0);
+			report_interval = (time_t)strtol(optarg, NULL, 0);
 			break;
 		case 'e':
 			report_corrected = true;
-- 
2.42.0

Re: [PATCH 2/5] xfs_repair: make duration take time_t

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 02:34:24PM +0200, Andrey Albershteyn wrote:
> In most of the uses of duration() takes time_t instead of int.
> Convert the rest to use time_t and make duration() take time_t to
> not truncate it to int.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  repair/progress.c   | 4 ++--
>  repair/progress.h   | 2 +-
>  repair/xfs_repair.c | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/repair/progress.c b/repair/progress.c
> index f6c4d988444e..c2af1387eb14 100644
> --- a/repair/progress.c
> +++ b/repair/progress.c
> @@ -273,7 +273,7 @@ progress_rpt_thread (void *p)
>  	_("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"),
>  				tmp->tm_hour, tmp->tm_min, tmp->tm_sec,
>  				current_phase, percent,
> -				duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));
> +				duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));

I'm not in love with how this expression mixes uint64_t and time_t, but
I guess it's all the same now that we forced time_t to 64 bits.

You might remove the pointless parentheses around elapsed.

>  		}
>  
>  		if (pthread_mutex_unlock(&msgp->mutex) != 0) {
> @@ -420,7 +420,7 @@ timestamp(int end, int phase, char *buf)
>  }
>  
>  char *
> -duration(int length, char *buf)
> +duration(time_t length, char *buf)
>  {
>  	int sum;
>  	int weeks;
> diff --git a/repair/progress.h b/repair/progress.h
> index 2c1690db1b17..9575df164aa0 100644
> --- a/repair/progress.h
> +++ b/repair/progress.h
> @@ -38,7 +38,7 @@ extern void summary_report(void);
>  extern int  set_progress_msg(int report, uint64_t total);
>  extern uint64_t print_final_rpt(void);
>  extern char *timestamp(int end, int phase, char *buf);
> -extern char *duration(int val, char *buf);
> +extern char *duration(time_t val, char *buf);
>  extern int do_parallel;
>  
>  #define	PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b)
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index ba9d28330d82..78a7205f0054 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -377,7 +377,7 @@ process_args(int argc, char **argv)
>  			do_prefetch = 0;
>  			break;
>  		case 't':
> -			report_interval = (int)strtol(optarg, NULL, 0);
> +			report_interval = (time_t)strtol(optarg, NULL, 0);

report_interval is declared as an int and this patch doesn't change
that.  <confused>

--D

>  			break;
>  		case 'e':
>  			report_corrected = true;
> -- 
> 2.42.0
> 
> 

Re: [PATCH 2/5] xfs_repair: make duration take time_t

[Andrey Albershteyn]

On 2024-04-16 09:12:30, Darrick J. Wong wrote:
> On Tue, Apr 16, 2024 at 02:34:24PM +0200, Andrey Albershteyn wrote:
> > In most of the uses of duration() takes time_t instead of int.
> > Convert the rest to use time_t and make duration() take time_t to
> > not truncate it to int.
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> >  repair/progress.c   | 4 ++--
> >  repair/progress.h   | 2 +-
> >  repair/xfs_repair.c | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/repair/progress.c b/repair/progress.c
> > index f6c4d988444e..c2af1387eb14 100644
> > --- a/repair/progress.c
> > +++ b/repair/progress.c
> > @@ -273,7 +273,7 @@ progress_rpt_thread (void *p)
> >  	_("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"),
> >  				tmp->tm_hour, tmp->tm_min, tmp->tm_sec,
> >  				current_phase, percent,
> > -				duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));
> > +				duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));
> 
> I'm not in love with how this expression mixes uint64_t and time_t, but
> I guess it's all the same now that we forced time_t to 64 bits.
> 
> You might remove the pointless parentheses around elapsed.

sure

> 
> >  		}
> >  
> >  		if (pthread_mutex_unlock(&msgp->mutex) != 0) {
> > @@ -420,7 +420,7 @@ timestamp(int end, int phase, char *buf)
> >  }
> >  
> >  char *
> > -duration(int length, char *buf)
> > +duration(time_t length, char *buf)
> >  {
> >  	int sum;
> >  	int weeks;
> > diff --git a/repair/progress.h b/repair/progress.h
> > index 2c1690db1b17..9575df164aa0 100644
> > --- a/repair/progress.h
> > +++ b/repair/progress.h
> > @@ -38,7 +38,7 @@ extern void summary_report(void);
> >  extern int  set_progress_msg(int report, uint64_t total);
> >  extern uint64_t print_final_rpt(void);
> >  extern char *timestamp(int end, int phase, char *buf);
> > -extern char *duration(int val, char *buf);
> > +extern char *duration(time_t val, char *buf);
> >  extern int do_parallel;
> >  
> >  #define	PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b)
> > diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> > index ba9d28330d82..78a7205f0054 100644
> > --- a/repair/xfs_repair.c
> > +++ b/repair/xfs_repair.c
> > @@ -377,7 +377,7 @@ process_args(int argc, char **argv)
> >  			do_prefetch = 0;
> >  			break;
> >  		case 't':
> > -			report_interval = (int)strtol(optarg, NULL, 0);
> > +			report_interval = (time_t)strtol(optarg, NULL, 0);
> 
> report_interval is declared as an int and this patch doesn't change
> that.  <confused>

ops, yeah, missed that. Will change it to report_interval

> 
> --D
> 
> >  			break;
> >  		case 'e':
> >  			report_corrected = true;
> > -- 
> > 2.42.0
> > 
> > 
> 

-- 
- Andrey

Re: [PATCH 2/5] xfs_repair: make duration take time_t

[Christoph Hellwig]

On Tue, Apr 16, 2024 at 02:34:24PM +0200, Andrey Albershteyn wrote:
> @@ -273,7 +273,7 @@ progress_rpt_thread (void *p)
>  	_("\t- %02d:%02d:%02d: Phase %d: %" PRIu64 "%% done - estimated remaining time %s\n"),
>  				tmp->tm_hour, tmp->tm_min, tmp->tm_sec,
>  				current_phase, percent,
> -				duration((int) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));
> +				duration((time_t) ((*msgp->total - sum) * (elapsed)/sum), msgbuf));

What is the point of the time_t cast the gets applied to the
final calculated expression?  Even if time_t is wieder than what
it was, it would only extent it after we've overlflow the original
type.  The whole thing also just is formatted and filled with useles
braces to make it look really odd.  Something like:

				current_phase, percent,
				duration((*msgp->total - sum) * elapsed / sum,
					msgbuf));

would be way more readable.

[PATCH 3/5] xfs_io: init count to stop loop from execution

[Andrey Albershteyn]

jdm_parentpaths() doesn't initialize count. If count happens to be
non-zero, following loop can result in access overflow.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 io/parent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/parent.c b/io/parent.c
index 8f63607ffec2..5750d98a3b75 100644
--- a/io/parent.c
+++ b/io/parent.c
@@ -112,7 +112,7 @@ check_parents(parent_t *parentbuf, size_t *parentbuf_size,
 	     jdm_fshandle_t *fshandlep, struct xfs_bstat *statp)
 {
 	int error, i;
-	__u32 count;
+	__u32 count = 0;
 	parent_t *entryp;
 
 	do {
-- 
2.42.0

Re: [PATCH 3/5] xfs_io: init count to stop loop from execution

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 02:34:25PM +0200, Andrey Albershteyn wrote:
> jdm_parentpaths() doesn't initialize count. If count happens to be
> non-zero, following loop can result in access overflow.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  io/parent.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/io/parent.c b/io/parent.c
> index 8f63607ffec2..5750d98a3b75 100644
> --- a/io/parent.c
> +++ b/io/parent.c
> @@ -112,7 +112,7 @@ check_parents(parent_t *parentbuf, size_t *parentbuf_size,

check_parents is an artifact of the old sgi parent pointers code and
(apparently) its need to check parent pointer correctness via xfs_io
commands.  The Linux parent pointers patchset fixed all those
referential integrity problems (thanks, Allison!) and will blow this
away, so I think we should ignore this report:

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/commit/io/parent.c?h=pptrs&id=c0854b85c1e8c90ea3eea930a20d1323e61ddb40

--D

>  	     jdm_fshandle_t *fshandlep, struct xfs_bstat *statp)
>  {
>  	int error, i;
> -	__u32 count;
> +	__u32 count = 0;
>  	parent_t *entryp;
>  
>  	do {
> -- 
> 2.42.0
> 
> 

Re: [PATCH 3/5] xfs_io: init count to stop loop from execution

[Andrey Albershteyn]

On 2024-04-16 09:15:30, Darrick J. Wong wrote:
> On Tue, Apr 16, 2024 at 02:34:25PM +0200, Andrey Albershteyn wrote:
> > jdm_parentpaths() doesn't initialize count. If count happens to be
> > non-zero, following loop can result in access overflow.
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> >  io/parent.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/io/parent.c b/io/parent.c
> > index 8f63607ffec2..5750d98a3b75 100644
> > --- a/io/parent.c
> > +++ b/io/parent.c
> > @@ -112,7 +112,7 @@ check_parents(parent_t *parentbuf, size_t *parentbuf_size,
> 
> check_parents is an artifact of the old sgi parent pointers code and
> (apparently) its need to check parent pointer correctness via xfs_io
> commands.  The Linux parent pointers patchset fixed all those
> referential integrity problems (thanks, Allison!) and will blow this
> away, so I think we should ignore this report:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/commit/io/parent.c?h=pptrs&id=c0854b85c1e8c90ea3eea930a20d1323e61ddb40

I see, thanks, will drop this one

> 
> --D
> 
> >  	     jdm_fshandle_t *fshandlep, struct xfs_bstat *statp)
> >  {
> >  	int error, i;
> > -	__u32 count;
> > +	__u32 count = 0;
> >  	parent_t *entryp;
> >  
> >  	do {
> > -- 
> > 2.42.0
> > 
> > 
> 

-- 
- Andrey

[PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized

[Andrey Albershteyn]

If unicrash_load() fails, all_pi can be used uninitialized in
phase_end(). Fix it by going to the unload: section if unicrash_load
fails and just go with unicrash_unload() (the is_service won't be
initialized here).

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 scrub/xfs_scrub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
index 752180d646ba..d226721d1dd7 100644
--- a/scrub/xfs_scrub.c
+++ b/scrub/xfs_scrub.c
@@ -631,7 +631,7 @@ main(
 		fprintf(stderr,
 	_("%s: couldn't initialize Unicode library.\n"),
 				progname);
-		goto out;
+		goto unload;
 	}
 
 	pthread_mutex_init(&ctx.lock, NULL);
@@ -828,6 +828,7 @@ out:
 	phase_end(&all_pi, 0);
 	if (progress_fp)
 		fclose(progress_fp);
+unload:
 	unicrash_unload();
 
 	/*
-- 
2.42.0

Re: [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 02:34:26PM +0200, Andrey Albershteyn wrote:
> If unicrash_load() fails, all_pi can be used uninitialized in
> phase_end(). Fix it by going to the unload: section if unicrash_load
> fails and just go with unicrash_unload() (the is_service won't be
> initialized here).
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  scrub/xfs_scrub.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 752180d646ba..d226721d1dd7 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -631,7 +631,7 @@ main(
>  		fprintf(stderr,
>  	_("%s: couldn't initialize Unicode library.\n"),
>  				progname);
> -		goto out;
> +		goto unload;

Rename this label out_unicrash to describe what piece will be unloaded,
and you can add:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  	}
>  
>  	pthread_mutex_init(&ctx.lock, NULL);
> @@ -828,6 +828,7 @@ out:
>  	phase_end(&all_pi, 0);
>  	if (progress_fp)
>  		fclose(progress_fp);
> +unload:
>  	unicrash_unload();
>  
>  	/*
> -- 
> 2.42.0
> 
> 

[PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int

[Andrey Albershteyn]

Convert howlong argument to a time_t as it's truncated to int, but in
practice this is not an issue as duration will never be this big.

Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 fsr/xfs_fsr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 3077d8f4ef46..07f3c8e23deb 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -72,7 +72,7 @@ static int  packfile(char *fname, char *tname, int fd,
 static void fsrdir(char *dirname);
 static int  fsrfs(char *mntdir, xfs_ino_t ino, int targetrange);
 static void initallfs(char *mtab);
-static void fsrallfs(char *mtab, int howlong, char *leftofffile);
+static void fsrallfs(char *mtab, time_t howlong, char *leftofffile);
 static void fsrall_cleanup(int timeout);
 static int  getnextents(int);
 int xfsrtextsize(int fd);
@@ -387,7 +387,7 @@ initallfs(char *mtab)
 }
 
 static void
-fsrallfs(char *mtab, int howlong, char *leftofffile)
+fsrallfs(char *mtab, time_t howlong, char *leftofffile)
 {
 	int fd;
 	int error;
-- 
2.42.0

Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 02:34:27PM +0200, Andrey Albershteyn wrote:
> Convert howlong argument to a time_t as it's truncated to int, but in
> practice this is not an issue as duration will never be this big.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  fsr/xfs_fsr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 3077d8f4ef46..07f3c8e23deb 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -72,7 +72,7 @@ static int  packfile(char *fname, char *tname, int fd,
>  static void fsrdir(char *dirname);
>  static int  fsrfs(char *mntdir, xfs_ino_t ino, int targetrange);
>  static void initallfs(char *mtab);
> -static void fsrallfs(char *mtab, int howlong, char *leftofffile);
> +static void fsrallfs(char *mtab, time_t howlong, char *leftofffile);
>  static void fsrall_cleanup(int timeout);
>  static int  getnextents(int);
>  int xfsrtextsize(int fd);
> @@ -387,7 +387,7 @@ initallfs(char *mtab)
>  }
>  
>  static void
> -fsrallfs(char *mtab, int howlong, char *leftofffile)
> +fsrallfs(char *mtab, time_t howlong, char *leftofffile)

Do you have to convert the printf format specifier too?

Also what happens if there's a parsing error and atoi() fails?  Right
now it looks like -t garbage gets you a zero run-time instead of a cli
parsing complaint?

--D

>  {
>  	int fd;
>  	int error;
> -- 
> 2.42.0
> 
> 

Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int

[Andrey Albershteyn]

On 2024-04-16 09:21:25, Darrick J. Wong wrote:
> On Tue, Apr 16, 2024 at 02:34:27PM +0200, Andrey Albershteyn wrote:
> > Convert howlong argument to a time_t as it's truncated to int, but in
> > practice this is not an issue as duration will never be this big.
> > 
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> >  fsr/xfs_fsr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index 3077d8f4ef46..07f3c8e23deb 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -72,7 +72,7 @@ static int  packfile(char *fname, char *tname, int fd,
> >  static void fsrdir(char *dirname);
> >  static int  fsrfs(char *mntdir, xfs_ino_t ino, int targetrange);
> >  static void initallfs(char *mtab);
> > -static void fsrallfs(char *mtab, int howlong, char *leftofffile);
> > +static void fsrallfs(char *mtab, time_t howlong, char *leftofffile);
> >  static void fsrall_cleanup(int timeout);
> >  static int  getnextents(int);
> >  int xfsrtextsize(int fd);
> > @@ -387,7 +387,7 @@ initallfs(char *mtab)
> >  }
> >  
> >  static void
> > -fsrallfs(char *mtab, int howlong, char *leftofffile)
> > +fsrallfs(char *mtab, time_t howlong, char *leftofffile)
> 
> Do you have to convert the printf format specifier too?

is time_t always long?

> 
> Also what happens if there's a parsing error and atoi() fails?  Right
> now it looks like -t garbage gets you a zero run-time instead of a cli
> parsing complaint?

I suppose it the same as atoi() returns 0 on garbage

> 
> --D
> 
> >  {
> >  	int fd;
> >  	int error;
> > -- 
> > 2.42.0
> > 
> > 
> 

-- 
- Andrey

Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 06:31:57PM +0200, Andrey Albershteyn wrote:
> On 2024-04-16 09:21:25, Darrick J. Wong wrote:
> > On Tue, Apr 16, 2024 at 02:34:27PM +0200, Andrey Albershteyn wrote:
> > > Convert howlong argument to a time_t as it's truncated to int, but in
> > > practice this is not an issue as duration will never be this big.
> > > 
> > > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > > ---
> > >  fsr/xfs_fsr.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > > index 3077d8f4ef46..07f3c8e23deb 100644
> > > --- a/fsr/xfs_fsr.c
> > > +++ b/fsr/xfs_fsr.c
> > > @@ -72,7 +72,7 @@ static int  packfile(char *fname, char *tname, int fd,
> > >  static void fsrdir(char *dirname);
> > >  static int  fsrfs(char *mntdir, xfs_ino_t ino, int targetrange);
> > >  static void initallfs(char *mtab);
> > > -static void fsrallfs(char *mtab, int howlong, char *leftofffile);
> > > +static void fsrallfs(char *mtab, time_t howlong, char *leftofffile);
> > >  static void fsrall_cleanup(int timeout);
> > >  static int  getnextents(int);
> > >  int xfsrtextsize(int fd);
> > > @@ -387,7 +387,7 @@ initallfs(char *mtab)
> > >  }
> > >  
> > >  static void
> > > -fsrallfs(char *mtab, int howlong, char *leftofffile)
> > > +fsrallfs(char *mtab, time_t howlong, char *leftofffile)
> > 
> > Do you have to convert the printf format specifier too?
> 
> is time_t always long?

There don't seem to be any guarantees at all.

The most portable strategy is to cast the value to an unsigned long long
and use %ll[ux].  Awkwardly, time_t seems to get used both for actual
timestamps and time deltas.

> > 
> > Also what happens if there's a parsing error and atoi() fails?  Right
> > now it looks like -t garbage gets you a zero run-time instead of a cli
> > parsing complaint?
> 
> I suppose it the same as atoi() returns 0 on garbage

<nod> All those cli integer parsing things need better checking, though
that's its own cleanup series and not related to this patch.

--D

> > 
> > --D
> > 
> > >  {
> > >  	int fd;
> > >  	int error;
> > > -- 
> > > 2.42.0
> > > 
> > > 
> > 
> 
> -- 
> - Andrey
> 
> 

Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int

[Eric Sandeen]

On 4/16/24 11:21 AM, Darrick J. Wong wrote:
> On Tue, Apr 16, 2024 at 02:34:27PM +0200, Andrey Albershteyn wrote:
>> Convert howlong argument to a time_t as it's truncated to int, but in
>> practice this is not an issue as duration will never be this big.
>>
>> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
>> ---
>>  fsr/xfs_fsr.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
>> index 3077d8f4ef46..07f3c8e23deb 100644
>> --- a/fsr/xfs_fsr.c
>> +++ b/fsr/xfs_fsr.c
>> @@ -72,7 +72,7 @@ static int  packfile(char *fname, char *tname, int fd,
>>  static void fsrdir(char *dirname);
>>  static int  fsrfs(char *mntdir, xfs_ino_t ino, int targetrange);
>>  static void initallfs(char *mtab);
>> -static void fsrallfs(char *mtab, int howlong, char *leftofffile);
>> +static void fsrallfs(char *mtab, time_t howlong, char *leftofffile);
>>  static void fsrall_cleanup(int timeout);
>>  static int  getnextents(int);
>>  int xfsrtextsize(int fd);
>> @@ -387,7 +387,7 @@ initallfs(char *mtab)
>>  }
>>  
>>  static void
>> -fsrallfs(char *mtab, int howlong, char *leftofffile)
>> +fsrallfs(char *mtab, time_t howlong, char *leftofffile)
> 
> Do you have to convert the printf format specifier too?

Hm, yeah. Another approach might be to just change "howlong"
to an int and reject run requests of more than 24855 days. ;)

*shrug* either way.

> Also what happens if there's a parsing error and atoi() fails?  Right
> now it looks like -t garbage gets you a zero run-time instead of a cli
> parsing complaint?

That seems like a buglet, but unrelated to the issue at hand, right?
So another patch, perhaps (using strtoul instead of atoi which can't
actually fail and just returns 0, if I remember correctly.)

(sorry about the navel-gazing over coverity here, s3kuret3h folks have
prioritized some "findings" and sometimes it's easier to tidy up
small issues like this than to argue. Thanks for the reviews!)

Thanks,
-Eric

> --D
> 
>>  {
>>  	int fd;
>>  	int error;
>> -- 
>> 2.42.0
>>
>>
> 

Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int

[Christoph Hellwig]

On Tue, Apr 16, 2024 at 11:39:14AM -0500, Eric Sandeen wrote:
> >> +fsrallfs(char *mtab, time_t howlong, char *leftofffile)
> > 
> > Do you have to convert the printf format specifier too?
> 
> Hm, yeah. Another approach might be to just change "howlong"
> to an int and reject run requests of more than 24855 days. ;)

That feels easier.  Especially when you have to deal with format
specifiers of ny kind sticking to "simple" types makes life way
easier.

> 
> *shrug* either way.
> 
> > Also what happens if there's a parsing error and atoi() fails?  Right
> > now it looks like -t garbage gets you a zero run-time instead of a cli
> > parsing complaint?
> 
> That seems like a buglet, but unrelated to the issue at hand, right?
> So another patch, perhaps (using strtoul instead of atoi which can't
> actually fail and just returns 0, if I remember correctly.)

Yeah, getting rid of atoi is always a good thing.

---end quoted text---