* [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan
@ 2024-04-16 20:23 Andrey Albershteyn
2024-04-16 20:23 ` [PATCH v2 1/4] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 20:23 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/4] xfs_db: fix leak in flist_find_ftyp()
2024-04-16 20:23 [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
@ 2024-04-16 20:23 ` Andrey Albershteyn
2024-04-16 20:24 ` [PATCH v2 2/4] xfs_repair: make duration take time_t Andrey Albershteyn
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 20:23 UTC (permalink / raw
To: cem, linux-xfs; +Cc: Andrey Albershteyn, Darrick J . Wong
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/4] xfs_repair: make duration take time_t
2024-04-16 20:23 [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
2024-04-16 20:23 ` [PATCH v2 1/4] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
@ 2024-04-16 20:24 ` Andrey Albershteyn
2024-04-16 21:00 ` Darrick J. Wong
2024-04-16 20:24 ` [PATCH v2 3/4] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 20:24 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/4] xfs_scrub: don't call phase_end if phase_rusage was not initialized
2024-04-16 20:23 [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
2024-04-16 20:23 ` [PATCH v2 1/4] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
2024-04-16 20:24 ` [PATCH v2 2/4] xfs_repair: make duration take time_t Andrey Albershteyn
@ 2024-04-16 20:24 ` Andrey Albershteyn
2024-04-16 20:24 ` [PATCH v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
2024-04-16 20:51 ` [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Bill O'Donnell
4 siblings, 0 replies; 8+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 20:24 UTC (permalink / raw
To: cem, linux-xfs; +Cc: Andrey Albershteyn, Darrick J . Wong
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 20:23 [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
` (2 preceding siblings ...)
2024-04-16 20:24 ` [PATCH v2 3/4] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
@ 2024-04-16 20:24 ` Andrey Albershteyn
2024-04-16 20:59 ` Darrick J. Wong
2024-04-16 20:51 ` [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Bill O'Donnell
4 siblings, 1 reply; 8+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 20:24 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan
2024-04-16 20:23 [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
` (3 preceding siblings ...)
2024-04-16 20:24 ` [PATCH v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
@ 2024-04-16 20:51 ` Bill O'Donnell
4 siblings, 0 replies; 8+ messages in thread
From: Bill O'Donnell @ 2024-04-16 20:51 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 20:24 ` [PATCH v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
@ 2024-04-16 20:59 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2024-04-16 20:59 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/4] xfs_repair: make duration take time_t
2024-04-16 20:24 ` [PATCH v2 2/4] xfs_repair: make duration take time_t Andrey Albershteyn
@ 2024-04-16 21:00 ` Darrick J. Wong
0 siblings, 0 replies; 8+ messages in thread
From: Darrick J. Wong @ 2024-04-16 21:00 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-16 21:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-16 20:23 [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
2024-04-16 20:23 ` [PATCH v2 1/4] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
2024-04-16 20:24 ` [PATCH v2 2/4] xfs_repair: make duration take time_t Andrey Albershteyn
2024-04-16 21:00 ` Darrick J. Wong
2024-04-16 20:24 ` [PATCH v2 3/4] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
2024-04-16 20:24 ` [PATCH v2 4/4] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
2024-04-16 20:59 ` Darrick J. Wong
2024-04-16 20:51 ` [PATCH v2 0/4] xfsprogs random fixes found by Coverity scan Bill O'Donnell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).