linux-kernel - Re: [Intel-gfx] [PATCH] i915/intel_tv_get_modes: fix strncpy truncation warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180712141015.GD5565@intel.com>
Date:   Thu, 12 Jul 2018 17:10:15 +0300
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Dominique Martinet <asmadeus@...ewreck.org>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        David Airlie <airlied@...ux.ie>,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] i915/intel_tv_get_modes: fix strncpy
 truncation warning

On Thu, Jul 12, 2018 at 03:55:26PM +0200, Dominique Martinet wrote:
> Ville Syrjälä wrote on Thu, Jul 12, 2018:
> > On Wed, Jul 11, 2018 at 09:46:15AM +0200, Dominique Martinet wrote:
> > > This is effectively no-op as the next line writes a nul at the final
> > 
> > What is "This". Please write self contained commit messages.
> 
> This could either be 'this commit' as a whole or if you look only at the
> commit message 'this strncpy fix' from the title (which is arguably the
> same), and both interpretations sound fairly understandable in the
> context of the title line without seeing the patch to me... Although
> I'll admit this is difficult to judge of that as the author.

The patch subject is not part of the commit message body though. This is
made all the more clear when I'm editing the response in vim that doesn't
even show the mail subject to me. Hence I'm always left in the dark by
commit messages that aren't fully self contained.

> 
> Thanksfully, the v2 of the patch didn't use this wording but while I
> agree the message could be better I do not think it was horrible.
> 
> 
> > > drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> > > drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals destination size [-Werror=stringop-truncation]
> > >    strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> > >    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > cc1: all warnings being treated as errors
> > 
> > That warning should be in the actual commit message.
> 
> Yes and no, I gave it for referrence but when you update to gcc 8 you
> will literally see it all over the place.
> The words "strncpy truncation warning" is really precise once you've
> seen them a few times and there are litteraly hundred of these warnings
> in the kernel, some have already been fixed taking a glance at the git
> log, some with and without the warning message.
> I don't think it's worth polluting the git log with this many
> warnings... Which leads to...

I disagree. Without knowing what exactly is fixed how can you judge 
whether the patch even makes sense? And later you may get another
report of the same warning and then you would want to look through
the git log to see if there's a patch that already fixed it. Quite
hard to do without the exact warning in the log.

-- 
Ville Syrjälä
Intel