Re: [PATCH ethtool] netlink: fix print_string when the value is NULL

[Salvatore Bonaccorso <carnil@debian.org>]

Hi,

On Mon, Aug 11, 2025 at 10:07:00PM +0200, Michal Kubecek wrote:
> Dne Sun, Aug 10, 2025 at 09:14:26AM GMT, Salvatore Bonaccorso napsal:
> > Hi Michal,
> > 
> > On Fri, Aug 08, 2025 at 01:05:52AM +0200, Michal Kubecek wrote:
> > > On Thu, Jul 24, 2025 at 07:48:11PM GMT, Michel Lind wrote:
> > > > The previous fix in commit b70c92866102 ("netlink: fix missing headers
> > > > in text output") handles the case when value is NULL by still using
> > > > `fprintf` but passing no value.
> > > > 
> > > > This fails if `-Werror=format-security` is passed to gcc, as is the
> > > > default in distros like Fedora.
> > > > 
> > > > ```
> > > > json_print.c: In function 'print_string':
> > > > json_print.c:147:25: error: format not a string literal and no format arguments [-Werror=format-security]
> > > >   147 |                         fprintf(stdout, fmt);
> > > >       |
> > > > ```
> > > > 
> > > > Use `fprintf(stdout, "%s", fmt)` instead, using the format string as the
> > > > value, since in this case we know it is just a string without format
> > > > chracters.
> > > > 
> > > > Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> > > > Signed-off-by: Michel Lind <michel@michel-slm.name>
> > > 
> > > Applied, thank you.
> > > 
> > > It's a bit surprising that I didn't hit this problem as I always test
> > > building with "-Wall -Wextra -Werror". I suppose this option is not
> > > contained in -Wall or -Wextra.
> > > 
> > > Michal
> > > 
> > > > ---
> > > >  json_print.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/json_print.c b/json_print.c
> > > > index e07c651..75e6cd9 100644
> > > > --- a/json_print.c
> > > > +++ b/json_print.c
> > > > @@ -144,7 +144,7 @@ void print_string(enum output_type type,
> > > >  		if (value)
> > > >  			fprintf(stdout, fmt, value);
> > > >  		else
> > > > -			fprintf(stdout, fmt);
> > > > +			fprintf(stdout, "%s", fmt);
> > > >  	}
> > > >  }
> > > >  
> > > > -- 
> > > > 2.50.1
> > 
> > As b70c92866102 ("netlink: fix missing headers in text output") was
> > backported as well for the 6.14.2 version, should that get as well a
> > new release 6.14.3 with the fix?
> 
> I could do that but it didn't seem necessary. If I understand correctly,
> this patch does not address any runtime issue (at least not until there
> is an actual call of print_string() with null value and fmt containing
> a template); and the build issue only happens with a very specific
> compiler option which is not only not default but is not included even
> in "-Wall -Wextra" (not even in gcc15).
> 
> I'm aware that the commit message says that Fedora uses that compiler
> option in its package builds but that's something that can be addressed
> by a distribution patch. Therefore my plan was to cherry pick the commit
> into ethtool-6.14.y branch but not to release 6.14.3 unless something
> more serious shows up.
> 
> But if I misunderstood the situation and 6.14.3 with this commit would
> be really helpful, I can reconsider.

No not urgent, but I hit the same issue when preparing 6.14.2 for
Debian trixie. But I can equally just cherry-pick the commit locally
and then drop it once 6.14.3 is released.

So really no hurry about that.

Regards,
Salvatore