| Message ID | 20240416123427.614899-6-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfsprogs random fixes found by Coverity scan | expand |
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 > >
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 > > > > >
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 >> >> >
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---
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 > >
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;
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(-)