* [PATCH 0/5] xfsprogs random fixes found by Coverity scan
@ 2024-04-16 12:34 Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 12:34 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp()
2024-04-16 12:34 [PATCH 0/5] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
@ 2024-04-16 12:34 ` Andrey Albershteyn
2024-04-16 16:07 ` Darrick J. Wong
2024-04-16 16:32 ` Christoph Hellwig
2024-04-16 12:34 ` [PATCH 2/5] xfs_repair: make duration take time_t Andrey Albershteyn
` (3 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 12:34 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/5] xfs_repair: make duration take time_t
2024-04-16 12:34 [PATCH 0/5] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
@ 2024-04-16 12:34 ` Andrey Albershteyn
2024-04-16 16:12 ` Darrick J. Wong
2024-04-16 16:45 ` Christoph Hellwig
2024-04-16 12:34 ` [PATCH 3/5] xfs_io: init count to stop loop from execution Andrey Albershteyn
` (2 subsequent siblings)
4 siblings, 2 replies; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 12:34 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.
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/5] xfs_io: init count to stop loop from execution
2024-04-16 12:34 [PATCH 0/5] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 2/5] xfs_repair: make duration take time_t Andrey Albershteyn
@ 2024-04-16 12:34 ` Andrey Albershteyn
2024-04-16 16:15 ` Darrick J. Wong
2024-04-16 12:34 ` [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
4 siblings, 1 reply; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 12:34 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized
2024-04-16 12:34 [PATCH 0/5] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
` (2 preceding siblings ...)
2024-04-16 12:34 ` [PATCH 3/5] xfs_io: init count to stop loop from execution Andrey Albershteyn
@ 2024-04-16 12:34 ` Andrey Albershteyn
2024-04-16 16:18 ` Darrick J. Wong
2024-04-16 12:34 ` [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
4 siblings, 1 reply; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 12:34 UTC (permalink / raw
To: cem, linux-xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 12:34 [PATCH 0/5] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
` (3 preceding siblings ...)
2024-04-16 12:34 ` [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
@ 2024-04-16 12:34 ` Andrey Albershteyn
2024-04-16 16:21 ` Darrick J. Wong
4 siblings, 1 reply; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 12:34 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.
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp()
2024-04-16 12:34 ` [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
@ 2024-04-16 16:07 ` Darrick J. Wong
2024-04-16 16:32 ` Christoph Hellwig
1 sibling, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2024-04-16 16:07 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] xfs_repair: make duration take time_t
2024-04-16 12:34 ` [PATCH 2/5] xfs_repair: make duration take time_t Andrey Albershteyn
@ 2024-04-16 16:12 ` Darrick J. Wong
2024-04-16 16:20 ` Andrey Albershteyn
2024-04-16 16:45 ` Christoph Hellwig
1 sibling, 1 reply; 19+ messages in thread
From: Darrick J. Wong @ 2024-04-16 16:12 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] xfs_io: init count to stop loop from execution
2024-04-16 12:34 ` [PATCH 3/5] xfs_io: init count to stop loop from execution Andrey Albershteyn
@ 2024-04-16 16:15 ` Darrick J. Wong
2024-04-16 16:22 ` Andrey Albershteyn
0 siblings, 1 reply; 19+ messages in thread
From: Darrick J. Wong @ 2024-04-16 16:15 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized
2024-04-16 12:34 ` [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
@ 2024-04-16 16:18 ` Darrick J. Wong
0 siblings, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2024-04-16 16:18 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] xfs_repair: make duration take time_t
2024-04-16 16:12 ` Darrick J. Wong
@ 2024-04-16 16:20 ` Andrey Albershteyn
0 siblings, 0 replies; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 16:20 UTC (permalink / raw
To: Darrick J. Wong; +Cc: cem, linux-xfs
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 12:34 ` [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
@ 2024-04-16 16:21 ` Darrick J. Wong
2024-04-16 16:31 ` Andrey Albershteyn
2024-04-16 16:39 ` Eric Sandeen
0 siblings, 2 replies; 19+ messages in thread
From: Darrick J. Wong @ 2024-04-16 16:21 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] xfs_io: init count to stop loop from execution
2024-04-16 16:15 ` Darrick J. Wong
@ 2024-04-16 16:22 ` Andrey Albershteyn
0 siblings, 0 replies; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 16:22 UTC (permalink / raw
To: Darrick J. Wong; +Cc: cem, linux-xfs
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 16:21 ` Darrick J. Wong
@ 2024-04-16 16:31 ` Andrey Albershteyn
2024-04-16 17:07 ` Darrick J. Wong
2024-04-16 16:39 ` Eric Sandeen
1 sibling, 1 reply; 19+ messages in thread
From: Andrey Albershteyn @ 2024-04-16 16:31 UTC (permalink / raw
To: Darrick J. Wong; +Cc: cem, linux-xfs
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp()
2024-04-16 12:34 ` [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
2024-04-16 16:07 ` Darrick J. Wong
@ 2024-04-16 16:32 ` Christoph Hellwig
1 sibling, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2024-04-16 16:32 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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..
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 16:21 ` Darrick J. Wong
2024-04-16 16:31 ` Andrey Albershteyn
@ 2024-04-16 16:39 ` Eric Sandeen
2024-04-16 16:41 ` Christoph Hellwig
1 sibling, 1 reply; 19+ messages in thread
From: Eric Sandeen @ 2024-04-16 16:39 UTC (permalink / raw
To: Darrick J. Wong, Andrey Albershteyn; +Cc: cem, linux-xfs
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
>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 16:39 ` Eric Sandeen
@ 2024-04-16 16:41 ` Christoph Hellwig
0 siblings, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2024-04-16 16:41 UTC (permalink / raw
To: Eric Sandeen; +Cc: Darrick J. Wong, Andrey Albershteyn, cem, linux-xfs
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---
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] xfs_repair: make duration take time_t
2024-04-16 12:34 ` [PATCH 2/5] xfs_repair: make duration take time_t Andrey Albershteyn
2024-04-16 16:12 ` Darrick J. Wong
@ 2024-04-16 16:45 ` Christoph Hellwig
1 sibling, 0 replies; 19+ messages in thread
From: Christoph Hellwig @ 2024-04-16 16:45 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int
2024-04-16 16:31 ` Andrey Albershteyn
@ 2024-04-16 17:07 ` Darrick J. Wong
0 siblings, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2024-04-16 17:07 UTC (permalink / raw
To: Andrey Albershteyn; +Cc: cem, linux-xfs
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
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-04-16 17:07 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-16 12:34 [PATCH 0/5] xfsprogs random fixes found by Coverity scan Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 1/5] xfs_db: fix leak in flist_find_ftyp() Andrey Albershteyn
2024-04-16 16:07 ` Darrick J. Wong
2024-04-16 16:32 ` Christoph Hellwig
2024-04-16 12:34 ` [PATCH 2/5] xfs_repair: make duration take time_t Andrey Albershteyn
2024-04-16 16:12 ` Darrick J. Wong
2024-04-16 16:20 ` Andrey Albershteyn
2024-04-16 16:45 ` Christoph Hellwig
2024-04-16 12:34 ` [PATCH 3/5] xfs_io: init count to stop loop from execution Andrey Albershteyn
2024-04-16 16:15 ` Darrick J. Wong
2024-04-16 16:22 ` Andrey Albershteyn
2024-04-16 12:34 ` [PATCH 4/5] xfs_scrub: don't call phase_end if phase_rusage was not initialized Andrey Albershteyn
2024-04-16 16:18 ` Darrick J. Wong
2024-04-16 12:34 ` [PATCH 5/5] xfs_fsr: convert fsrallfs to use time_t instead of int Andrey Albershteyn
2024-04-16 16:21 ` Darrick J. Wong
2024-04-16 16:31 ` Andrey Albershteyn
2024-04-16 17:07 ` Darrick J. Wong
2024-04-16 16:39 ` Eric Sandeen
2024-04-16 16:41 ` Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.