[PATCH] D17983: Eliminate many benign instances of "potentially uninitialized local variable" warnings

[David Blaikie via cfe-commits]

On Wed, Mar 9, 2016 at 11:58 AM, Alexander Riccio via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> ariccio added a comment.
>
> In http://reviews.llvm.org/D17983#370717, @dblaikie wrote:
>
> > Initializations we never expect to use (eg because we have a covered
> switch
> >  that initializes in all cases, or some slightly complex control flow the
> >  compiler can't see through) hinder our ability to find uses of those
> with
> >  tools like msan.
>
>
> Too bad... Maybe msan should have a way to specify that you're default
> initializing a variable, and treat it as if you're not initializing it at
> all?
>

Maybe - could be worth discussing (I'd start a more general thread for that
if I were you, with some msan devs and llvm-dev)


> That said, I'm not sure that I agree with keeping `Potentially
> uninitialized local pointer variable 'name' used` off (//that's C4703 <
> https://msdn.microsoft.com/en-us/library/jj851030.aspx>, not C4701 <
> https://msdn.microsoft.com/en-us/library/1wea5zwe.aspx>//), because
> uninitialized local pointer bugs are more dangerous (I'm thinking <
> http://googleprojectzero.blogspot.com/2015/08/one-font-vulnerability-to-rule-them-all_21.html>
> of write-what-where <
> http://thesprawl.org/research/ost-introduction-software-exploits-uninit-overflow/>
> uninitialized pointer vulnerabilities <
> http://ioctl.ir/index.php/2016/02/13/cve-2016-0040-story-of-uninitialized-pointer/>),
> and I'd rather crash on a nullptr deref than some undefined behavior.
>

That being the problem - UB is what allows us to check these things and
have confidence we're finding bugs. (& not all the code, of course, would
deref into null - some places might use null as a valid state that is only
established after initialization (eg: int *x; if (a) { x = nullptr; } - so
we could still catch use of uninitialized in addition to null pointer
handling)


> I will drop the `default` cases as @dsanders noted, but should I drop all
> the local variable inits too? In several cases (which I didn't specifically
> note) I chose default initialization values that would trigger pre-existing
> `assert`s if nobody assigned anything else to them.
>

I'm not sure how valuable that is - again, we have msan to catch these and
broader issues.


> Regarding the 33 warnings I noted, i couldn't tell whether they're
> obviously false positives, which I think warrants someone other than me
> carefully looking through it.


These are 33 warnings you don't have fixes for? Sorry, I haven't looked
closely at the warning list compared to the proposed patch.


> Some of the warnings are caused by assigning to a variable inside of a
> loop with a dynamically sized bound; for those, I'd much rather (a)
> `assert` that the bound is nonzero/loop will be executed at least once, or
> (b) assign the variable some sentinel value, and then assert it before the
> variable is actually used. Thoughts?
>

I'd be happy to see an assertion for a loop like that, generally. (though I
haven't looked at the specific cases you're referring to)

- Dave


>
>
> http://reviews.llvm.org/D17983
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160309/31921f10/attachment.html>