I used David-Sarah's suggestion. See http://codereview.appspot.com/107058
2009/8/31 Mike Samuel <[email protected]>: > 2009/8/31 Mark Miller <[email protected]>: >> >> Forwarded by request. >> >> >> ---------- Forwarded message ---------- >> From: David Wagner <[email protected]> >> Date: Mon, Aug 31, 2009 at 4:47 PM >> Subject: Re: [Caja] Re: using google collections to simplify code >> To: Mark Miller <[email protected]> >> Cc: [email protected], Adrian Mettler >> <[email protected]>, Tyler Close <[email protected]>, David >> Wagner <[email protected]> >> >> >>> It came up quite painfully during the Waterken security review. When >>> reviewing code, it's really hard to read ConstList<Foo> and to >>> remember that the list may actually contain non-Foo's. The basic rule >>> we had to train ourselves in is (with a few exceptions): don't believe >>> anything between angle brackets. It may be lying to you. Erasing these >>> from the source text would have helped us erase them from our heads. >> >> Personal opinion: I didn't remember this being very painful for me. >> Like you say, I trained myself not to believe anything inside the angle >> brackets. At that point, I don't think I thought about it any more. >> As a code reviewer, I don't remember this being a big deal for me. >> >> Just one data point; others' experience may well have been different. >> (It's possible that I'm unusually aware of this because Adrian and I >> spent a bunch of time thinking about it when we designed Joe-E, so I >> may not be a representative data point.) >> >>> We worried less about the randomization of the sources of >>> ClassCastExceptions since we were generally assuming that anything one >>> called may throw any unchecked exception at any time. Joe-E does >>> enable much tighter reasoning about catchable exceptions, including >>> ClassCastExceptions, since the errors that may be throws at any time, >>> such as OutOfMemoryError, are not catchable in Joe-E. Indeed, Joe-E >>> enforces that throwing any Error is immediately fatal. Were we to make >>> use of those tighter reasoning abilities, the randomization of >>> ClassCastExceptions would become a painful hazard. >> >> Even in Joe-E, I think you still usually want to assume that anything you >> call can throw any unchecked exception at any time. ClassCastExceptions >> are one example of an unchecked exception, so when reviewing code you >> assume (conservatively) that ClassCastExceptions can be thrown anywhere, >> like any other unchecked assumption. That seems OK. >> >> Does Joe-E change anything here? I don't see why it would. Joe-E >> enforces that Errors are uncatchable, but ClassCastException is not >> an Error. Joe-E's tighter reasoning about Errors doesn't help reason >> about unchecked exceptions, does it? I don't see why randomization >> of ClassCastExceptions would become a bigger problem if you were to >> migrate to Joe-E. >> >> I think Ihab has an excellent point. The hazard with generics (you >> can't trust anything inside the angle brackets, due to the potential >> for heap pollution) is probably a bigger deal if you're dealing with >> malicious or untrusted code. For your case, where you're more worried >> about accidental bugs, maybe it's less of an issue? Perhaps you can to >> train yourself to follow these rules when reviewing code: >> >> 1. When reviewing code that receives a value whose declared type >> has angle brackets, don't believe anything inside the angle brackets. >> 2. When reviewing code that creates a value of some generic type, >> if it pollutes the heap (creates a value where the runtime type >> and declared type differ inside the angle brackets), then look >> extra suspiciously at that code. >> >> This may act as a belt-and-suspenders. For some inadvertent bug >> to create a security hole, you'd need the following 4 conditions to >> hold: >> >> (a) Someone would have to inadvertently introduce code that >> pollutes the heap. >> (b) The reviewer who examined the code in (a) would have to >> overlook the violation of rule 2. >> (c) Someone would have to inadvertently introduce code that >> trusts what's inside the angle brackets, i.e., that fails >> insecurely in the presence of heap pollution. >> (d) The reviewer who examined the code in (c) would have to >> overlook the violation of rule 1. >> >> Would that help? >> >> OK, there are subtleties around library code that uses >> @SuppressWarnings. Generally, the way I think about it is that >> if the @SuppressWarnings method can introduce heap pollution for >> the first time (assuming the heap wasn't already polluted before >> that method), then that's a real problem. It's a hazard that can >> conceal bugs, and reduces your belt-and-suspenders security to >> belt-and-thin-air security. >> >> But if the method can't introduce heap pollution itself, and is just >> using @SuppressWarnings to get around cases where the Java static type >> checker is too dumb to recognize that the code is OK, then I don't see >> any reason to worry about the presence of @SuppressWarnings. (I seem to >> recall some really annoying case where creation of an array of dynamic >> type required reflection and/or @SuppressWarnings, due to the >> inexpressiveness of Java's type system, but I don't recall the details.) > > This comes up a lot with generic var args methods. > Arrays.asList has a signature like > <T> static List<? extends T> asList(T... elements); > which means that when you call it with a generic type like > List<String> it has to create an array. > The element type for an array must not be generic, so instead of > creating a List<String>[] under the hood, it creates a List[] and you > get a missing type parameter warning. > > >> >> Put another way, if we imagine a thought experiment where Java did >> reify generic types at runtime (a la C#), then if you have a method >> that's marked @SuppressWarnings and that method could introduce heap >> pollution at runtime, then that method is problematic; but if that >> method couldn't possibly introduce heap pollution at runtime, then >> the use of @SuppressWarnings doesn't bother me and doesn't seem to >> invalidate the safety argument above. >> >> Does that make sense? Did I miss something? >> >> I have no idea whether adopting the Google Collections library >> would make it any harder to transition to Joe-E, which I guess is >> the core question. Sorry. >> >> >> >> -- >> Text by me above is hereby placed in the public domain >> >> Cheers, >> --MarkM >> >
