[PATCH] xfs_repair: detect null buf passed to duration

[PATCH] xfs_repair: detect null buf passed to duration

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

gcc 12.2 with ubsan and fortify turned on complains about this:

In file included from /usr/include/stdio.h:906,
                 from ../include/platform_defs.h:9,
                 from ../include/libxfs.h:16,
                 from progress.c:3:
In function ‘sprintf’,
    inlined from ‘duration’ at progress.c:443:4:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:30:10: error: null destination pointer [-Werror=format-overflow=]
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~

I think this is a false negative since all callers are careful not to
pass in a null pointer.  Unfortunately the compiler cannot detect that
since this isn't a static function and complains.  Fix this by adding an
explicit null check.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/progress.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/repair/progress.c b/repair/progress.c
index 084afa63c121..e13494e0ed23 100644
--- a/repair/progress.c
+++ b/repair/progress.c
@@ -435,6 +435,9 @@ duration(time_t length, char *buf)
 	int seconds;
 	char temp[128];
 
+	if (!buf)
+		return NULL;
+
 	*buf = '\0';
 	weeks = days = hours = minutes = seconds = sum = 0;
 	if (length >= ONEWEEK) {

Re: [PATCH] xfs_repair: detect null buf passed to duration

[Christoph Hellwig]

On Fri, May 31, 2024 at 01:10:39PM -0700, Darrick J. Wong wrote:
> I think this is a false negative since all callers are careful not to
> pass in a null pointer.

Yes.

> Unfortunately the compiler cannot detect that
> since this isn't a static function and complains.  Fix this by adding an
> explicit null check.

Can you try adding a __attribute__((nonnull(2))) to the declaration like
this?

diff --git a/repair/progress.h b/repair/progress.h
index 0b06b2c4f..c09aa6941 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(struct xfs_mount *mp, int end, int phase, char *buf);
-extern char *duration(time_t val, char *buf);
+char *duration(time_t val, char *buf) __attribute__((nonnull(2)));
 extern int do_parallel;
 
 #define	PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b)

Re: [PATCH] xfs_repair: detect null buf passed to duration

[Darrick J. Wong]

On Fri, May 31, 2024 at 10:00:25PM -0700, Christoph Hellwig wrote:
> On Fri, May 31, 2024 at 01:10:39PM -0700, Darrick J. Wong wrote:
> > I think this is a false negative since all callers are careful not to
> > pass in a null pointer.
> 
> Yes.
> 
> > Unfortunately the compiler cannot detect that
> > since this isn't a static function and complains.  Fix this by adding an
> > explicit null check.
> 
> Can you try adding a __attribute__((nonnull(2))) to the declaration like
> this?

Seems to work, I'll send in a v2.

--D

> diff --git a/repair/progress.h b/repair/progress.h
> index 0b06b2c4f..c09aa6941 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(struct xfs_mount *mp, int end, int phase, char *buf);
> -extern char *duration(time_t val, char *buf);
> +char *duration(time_t val, char *buf) __attribute__((nonnull(2)));
>  extern int do_parallel;
>  
>  #define	PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b)
> 

[PATCH v2] xfs_repair: detect null buf passed to duration

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

gcc 12.2 with ubsan and fortify turned on complains about this:

In file included from /usr/include/stdio.h:906,
                 from ../include/platform_defs.h:9,
                 from ../include/libxfs.h:16,
                 from progress.c:3:
In function ‘sprintf’,
    inlined from ‘duration’ at progress.c:443:4:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:30:10: error: null destination pointer [-Werror=format-overflow=]
   30 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |                                   __glibc_objsize (__s), __fmt,
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |                                   __va_arg_pack ());
      |                                   ~~~~~~~~~~~~~~~~~

I think this is a false negative since all callers are careful not to
pass in a null pointer.  Unfortunately the compiler cannot detect that
since this isn't a static function and complains.  Fix this by adding an
explicit declaration that buf isn't null.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/progress.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/repair/progress.h b/repair/progress.h
index 0b06b2c4f43f..c09aa69413ac 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(struct xfs_mount *mp, int end, int phase, char *buf);
-extern char *duration(time_t val, char *buf);
+char *duration(time_t val, char *buf) __attribute__((nonnull(2)));
 extern int do_parallel;
 
 #define	PROG_RPT_INC(a,b) if (ag_stride && prog_rpt_done) (a) += (b)

Re: [PATCH v2] xfs_repair: detect null buf passed to duration

[Carlos Maiolino]

> diff --git a/repair/progress.h b/repair/progress.h
> index 0b06b2c4f43f..c09aa69413ac 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(struct xfs_mount *mp, int end, int phase, char *buf);
> -extern char *duration(time_t val, char *buf);
> +char *duration(time_t val, char *buf) __attribute__((nonnull(2)));

Once nonnull() is used here, shouldn't we also set -Wnonnull to CFLAGS?

Please don't take it as a review, it's just a question that came to my mind as I don't fully
understand the implications of using nonnull here.

Carlos

Re: [PATCH v2] xfs_repair: detect null buf passed to duration

[Darrick J. Wong]

On Mon, Jun 03, 2024 at 02:42:20PM +0200, Carlos Maiolino wrote:
> 
> > diff --git a/repair/progress.h b/repair/progress.h
> > index 0b06b2c4f43f..c09aa69413ac 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(struct xfs_mount *mp, int end, int phase, char *buf);
> > -extern char *duration(time_t val, char *buf);
> > +char *duration(time_t val, char *buf) __attribute__((nonnull(2)));
> 
> Once nonnull() is used here, shouldn't we also set -Wnonnull to CFLAGS?

Already set via -Wall, at least if you're using gcc 12.2:

       -Wnonnull
           Warn about passing a null pointer for arguments marked as
           requiring a non-null value by the "nonnull" function
           attribute.

           -Wnonnull is included in -Wall and -Wformat.  It can be
           disabled with the -Wno-nonnull option.

https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/tree/include/builddefs.in?h=for-next#n113

> Please don't take it as a review, it's just a question that came to my mind as I don't fully
> understand the implications of using nonnull here.

<nod>

--D

> Carlos
> 

Re: [PATCH v2] xfs_repair: detect null buf passed to duration

[Carlos Maiolino]

On Mon, Jun 03, 2024 at 09:14:46AM GMT, Darrick J. Wong wrote:
> On Mon, Jun 03, 2024 at 02:42:20PM +0200, Carlos Maiolino wrote:
> >
> > > diff --git a/repair/progress.h b/repair/progress.h
> > > index 0b06b2c4f43f..c09aa69413ac 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(struct xfs_mount *mp, int end, int phase, char *buf);
> > > -extern char *duration(time_t val, char *buf);
> > > +char *duration(time_t val, char *buf) __attribute__((nonnull(2)));
> >
> > Once nonnull() is used here, shouldn't we also set -Wnonnull to CFLAGS?
> 
> Already set via -Wall, at least if you're using gcc 12.2:
> 
>        -Wnonnull
>            Warn about passing a null pointer for arguments marked as
>            requiring a non-null value by the "nonnull" function
>            attribute.
> 
>            -Wnonnull is included in -Wall and -Wformat.  It can be
>            disabled with the -Wno-nonnull option.

Ok, thanks for letting me know :) feel free to add:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/tree/include/builddefs.in?h=for-next#n113
> 
> > Please don't take it as a review, it's just a question that came to my mind as I don't fully
> > understand the implications of using nonnull here.
> 
> <nod>
> 
> --D
> 
> > Carlos
> >

Re: [PATCH v2] xfs_repair: detect null buf passed to duration

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>