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

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

[Andrey Albershteyn]

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 (4):
  xfs_db: fix leak in flist_find_ftyp()
  xfs_repair: make duration take time_t
  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       | 8 ++++++--
 repair/globals.c    | 2 +-
 repair/globals.h    | 2 +-
 repair/progress.c   | 7 ++++---
 repair/progress.h   | 2 +-
 repair/xfs_repair.c | 2 +-
 scrub/xfs_scrub.c   | 3 ++-
 8 files changed, 19 insertions(+), 11 deletions(-)

-- 
2.42.0

[PATCH v2 1/4] 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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

[PATCH v2 2/4] 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.

While at it remove unnecessary parentheses around 'elapsed'.

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

diff --git a/repair/globals.c b/repair/globals.c
index c40849853b8f..7c819d70a0ab 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -116,7 +116,7 @@ uint32_t	sb_width;
 struct aglock	*ag_locks;
 struct aglock	rt_lock;
 
-int		report_interval;
+time_t		report_interval;
 uint64_t	*prog_rpt_done;
 
 int		ag_stride;
diff --git a/repair/globals.h b/repair/globals.h
index 89f1b0e078f3..2d05c8b2c00f 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -160,7 +160,7 @@ struct aglock {
 extern struct aglock	*ag_locks;
 extern struct aglock	rt_lock;
 
-extern int		report_interval;
+extern time_t		report_interval;
 extern uint64_t		*prog_rpt_done;
 
 extern int		ag_stride;
diff --git a/repair/progress.c b/repair/progress.c
index f6c4d988444e..5f80fb68ddfd 100644
--- a/repair/progress.c
+++ b/repair/progress.c
@@ -268,12 +268,13 @@ progress_rpt_thread (void *p)
 				_("\t- %02d:%02d:%02d: Phase %d: elapsed time %s - processed %d %s per minute\n"),
 				tmp->tm_hour, tmp->tm_min, tmp->tm_sec,
 				current_phase, duration(elapsed, msgbuf),
-				(int) (60*sum/(elapsed)), *msgp->format->type);
+				(int) (60*sum/elapsed), *msgp->format->type);
 			do_log(
 	_("\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((*msgp->total - sum) * elapsed/sum,
+					msgbuf));
 		}
 
 		if (pthread_mutex_unlock(&msgp->mutex) != 0) {
@@ -420,7 +421,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..2ceea87dc57d 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 = strtol(optarg, NULL, 0);
 			break;
 		case 'e':
 			report_corrected = true;
-- 
2.42.0

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

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 10:24:00PM +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.
> 
> While at it remove unnecessary parentheses around 'elapsed'.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  repair/globals.c    | 2 +-
>  repair/globals.h    | 2 +-
>  repair/progress.c   | 7 ++++---
>  repair/progress.h   | 2 +-
>  repair/xfs_repair.c | 2 +-
>  5 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/repair/globals.c b/repair/globals.c
> index c40849853b8f..7c819d70a0ab 100644
> --- a/repair/globals.c
> +++ b/repair/globals.c
> @@ -116,7 +116,7 @@ uint32_t	sb_width;
>  struct aglock	*ag_locks;
>  struct aglock	rt_lock;
>  
> -int		report_interval;
> +time_t		report_interval;
>  uint64_t	*prog_rpt_done;
>  
>  int		ag_stride;
> diff --git a/repair/globals.h b/repair/globals.h
> index 89f1b0e078f3..2d05c8b2c00f 100644
> --- a/repair/globals.h
> +++ b/repair/globals.h
> @@ -160,7 +160,7 @@ struct aglock {
>  extern struct aglock	*ag_locks;
>  extern struct aglock	rt_lock;
>  
> -extern int		report_interval;
> +extern time_t		report_interval;
>  extern uint64_t		*prog_rpt_done;
>  
>  extern int		ag_stride;
> diff --git a/repair/progress.c b/repair/progress.c
> index f6c4d988444e..5f80fb68ddfd 100644
> --- a/repair/progress.c
> +++ b/repair/progress.c
> @@ -268,12 +268,13 @@ progress_rpt_thread (void *p)
>  				_("\t- %02d:%02d:%02d: Phase %d: elapsed time %s - processed %d %s per minute\n"),
>  				tmp->tm_hour, tmp->tm_min, tmp->tm_sec,
>  				current_phase, duration(elapsed, msgbuf),
> -				(int) (60*sum/(elapsed)), *msgp->format->type);
> +				(int) (60*sum/elapsed), *msgp->format->type);
>  			do_log(
>  	_("\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((*msgp->total - sum) * elapsed/sum,
> +					msgbuf));
>  		}
>  
>  		if (pthread_mutex_unlock(&msgp->mutex) != 0) {
> @@ -420,7 +421,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..2ceea87dc57d 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 = strtol(optarg, NULL, 0);

This also needs to clear and check errno to report errors.  However,
that should be a separate patch.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

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

[PATCH v2 3/4] 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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..50565857ddd8 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 out_unicrash;
 	}
 
 	pthread_mutex_init(&ctx.lock, NULL);
@@ -828,6 +828,7 @@ out:
 	phase_end(&all_pi, 0);
 	if (progress_fp)
 		fclose(progress_fp);
+out_unicrash:
 	unicrash_unload();
 
 	/*
-- 
2.42.0

[PATCH v2 4/4] 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.

Add check for howlong to fit into int (printf can use int format
specifier). Even longer interval doesn't make much sense.

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

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 3077d8f4ef46..4e29a8a2c548 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);
@@ -165,6 +165,10 @@ main(int argc, char **argv)
 			break;
 		case 't':
 			howlong = atoi(optarg);
+			if (howlong > INT_MAX) {
+				fprintf(stderr, _("%s: too long\n"), progname);
+				exit(1);
+			}
 			break;
 		case 'f':
 			leftofffile = optarg;
@@ -387,7 +391,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 v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int

[Darrick J. Wong]

On Tue, Apr 16, 2024 at 10:24:02PM +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.
> 
> Add check for howlong to fit into int (printf can use int format
> specifier). Even longer interval doesn't make much sense.
> 
> Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> ---
>  fsr/xfs_fsr.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 3077d8f4ef46..4e29a8a2c548 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);
> @@ -165,6 +165,10 @@ main(int argc, char **argv)
>  			break;
>  		case 't':
>  			howlong = atoi(optarg);
> +			if (howlong > INT_MAX) {
> +				fprintf(stderr, _("%s: too long\n"), progname);

Don't just say that it's too long; tell the user what the maximum is.
Also perhaps print the argument that was wrong so that the user knows
exactly what they got wrong:

	fprintf(stderr, _("%s: the maximum runtime is %d seconds.\n"),
			optarg, INT_MAX);

$ xfs_fsr -t 123456789123456789
123456789123456789: the maximum runtime is 2147483647 seconds.

--D

> +				exit(1);
> +			}
>  			break;
>  		case 'f':
>  			leftofffile = optarg;
> @@ -387,7 +391,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 v2 0/4] xfsprogs random fixes found by Coverity scan

[Bill O'Donnell]

On Tue, Apr 16, 2024 at 10:23:58PM +0200, Andrey Albershteyn wrote:
> 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

Could you add a brief change history on patch 0/4 for v2?
Besides that, the series looks fine to me and you can add:
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>

> 
> Andrey Albershteyn (4):
>   xfs_db: fix leak in flist_find_ftyp()
>   xfs_repair: make duration take time_t
>   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       | 8 ++++++--
>  repair/globals.c    | 2 +-
>  repair/globals.h    | 2 +-
>  repair/progress.c   | 7 ++++---
>  repair/progress.h   | 2 +-
>  repair/xfs_repair.c | 2 +-
>  scrub/xfs_scrub.c   | 3 ++-
>  8 files changed, 19 insertions(+), 11 deletions(-)
> 
> -- 
> 2.42.0
> 
>