Java: Remove requirements for final and access mods from DateFormatThreadUnsafe

[emilejq]

The rule checking for unsafe use of the DateFormat class in Java is incorrectly requiring that the DateFormat variable be declared final and either public or protected.

This PR removes those two requirements

[smowton]

I suspect these are here as heuristics -- after all even static-ness is just a heuristic; you can get this just as wrong by sharing between threads using instance fields or local variables with some way to gain a cross-thread alias.

Excluding private or package-private fields is likely acknowledging that at least then you control the accesses and might enforce some appropriate locking discipline or know your code is single-threaded.

On the other hand a final and non-final exposed field seem equally problematic to me, so I would support removing that constraint.

[emilejq]

I take the point about heuristics. I guess the risk with requiring the field to be private or protected is that it assumes someone falling foul of this rule is aware of the concurrency issues with the DateFormat class and has written the code accordingly.

At any rate, the reason I created this PR is because the example of non-compliant code given in the help files wouldn't be detected by the current implementation of this rule due to the two reasons given here. The field is declared private static DateFormat dateF = new SimpleDateFormat("yyyyMMdd");

https://github.com/github/codeql/blob/main/java/ql/src/Likely%20Bugs/Concurrency/DateFormatThreadUnsafe.java

[smowton]

Good point, please do update that to use a public field.

At the end of the day it's a tradeoff between nuisance alerts and missing ones; I don't see either clearly better than the other. Like I say if you want to change this to omit isFinal but keep the visibility constraint I'd merge that.

[emilejq]

Done