[PATCH weston 7/9] compositor-drm: fix uninitialized bytes on modeinfo

[Pekka Paalanen]

On Mon, 12 Feb 2018 12:59:22 +0000
Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
> 
> On 12 February 2018 at 12:51, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Mon, 12 Feb 2018 12:28:55 +0000 Daniel Stone <daniel at fooishbar.org> wrote:  
> >> Second and last nitpick: this is very clever, but should probably just
> >> use memset() like everywhere else.  
> >
> > I hate memset. :-)  
> 
> Me too!
> 
> > You have to explicitly give the size, and you have to remember in which
> > order the memset arguments should come. The above syntax may look a
> > little strange if you're not used to it, but I would much rather let
> > the compiler figure things out. We already use the same compiler
> > feature to initialize local struct variables, without the cast.
> >
> > It certainly takes me several brain cycles to check a memset, e.g. this
> > line has two bugs:
> >
> >         memset(mode, sizeof mode, 0);  
> 
> To be fair, GCC does warn about both of those, and has done since 4.x IIRC.
> 
> foo.c: In function ‘main’:
> foo.c:17:2: warning: ‘memset’ used with constant zero length
> parameter; this could be due to transposed parameters
> [-Wmemset-transposed-args]
>   memset(bar, sizeof bar, 0);
> 
> foo.c: In function ‘main’:
> foo.c:17:24: warning: argument to ‘sizeof’ in ‘memset’ call is the
> same expression as the destination; did you mean to dereference it?
> [-Wsizeof-pointer-memaccess]
>   memset(bar, 0, sizeof bar);

Oh, that's nice.

> > Would you take a patch to replace all memsets with the above assignment
> > form?
> >
> > I believe nothing is depending on the values of implicit padding bytes
> > in structs anyway, if such happen to exist and if compiler would happen
> > to leave them uninitialized.  
> 
> Hmm. On the other hand, if the ={} form only zeroes bytes accessible
> via members, might this caveat not defeat the purpose of the patch?
> IOW, if there is implicit padding not cleared by this assignment form,
> but something is copying the entire size of the struct, that may be
> treated as a read from uninitialised memory.

True. The same caveat applies to anything we initialize without a full
memset. I'd think it to be so common though that tools like Valgrind
might have special-casing there.

> > Or if you insist on memset, how about a macro:
> >
> > #define MEMCLEARPTR(x) memset((x), 0, sizeof *(x))  
> 
> I'd definitely be happy with that.

Even though I said it, I don't like it too much.

> I don't really mind which form we take, as long as it a) works and b)
> is consistent. Being harder to get wrong than raw memset is definitely
> a feature too. :)

Yeah, I'll go with memset for now, since the compiler warnings.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180212/903710c3/attachment.sig>