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

[Daniel Stone]

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);

> 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.

> 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.

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. :)

Cheers,
Daniel