About java.lang.String#format(java.lang.String,java.lang.Object[])

[Stephan202]

Having evaluated this plugin against our code base, I can say this is a job very well done. Kudos!

One issue I hit while using the bundled jdk-unsafe signatures is this violation:

[ERROR] Forbidden method invocation: java.lang.String#format(java.lang.String,java.lang.Object[]) [Uses default locale]

Indeed, this method can produce locale-dependent results. But in our code base I found over 400 calls to this method where the format pattern only contains %s format specifiers. As best as I can tell we never pass in objects which implement java.util.Formattable. So the results should be locale-independent; to vacuously add Locale.ROOT in hundreds of places seems overkill.

I guess making the plugin smart enough to detect this kind of safe method usage is out of scope. So my question is whether there is some way to exclude just this signature. If at all possible I'd like to avoid having to maintain a modified copy of the jdk-unsafe signature list.

[uschindler]

Hi,
sorry for the late reply (was very busy)!

At the moment, I am not sure how the best way is to make this smarter. Currently you can use @SuppressForbidden only, but that won't help for this use-case.

As forbiddenapis is only a static checker, there is no way to check for string contents (this is more something for tools like PMD). So I don't think that automatic exclusion here is fine. Especially, as %s can be applied to formattables.... On Lucene/Solr/Elasticsearch code we just added the Locale everywhere, although it's boilerplate.

One possibility would be some "exclusion" pattern (something like a negative signatures file) so you can slecetively revert patterns. For now, your only chance is to fork the signatures file into your project. BTW: You can also publish it on Maven Central or your local Artifactory repo and refer to it from your project's plugin and reuse it somewhere else!

[hakanai]

String.format has other shady things going on too. For instance, if you pass a Date, that will format using the default time zone of course. And for some reason, they didn't think it worth putting a timeZone() method on Formatter, so you can't even implement a Formattable correctly if you want it in another zone. >:-/

[Stephan202]

@trejkaz interesting :). That'd be an argument for adding Date#toString to one of the plugin's blacklists. Though in most cases (as in your example) the invocation of #toString is implicit 🤔 .

@uschindler apologies for not responding to your comment (also very busy here ;) ). I think adding support for exclusions would be a nice to have. When I get back to reviewing this plugin (could be weeks/months) I might look into contributing such support. Indeed, until that time I could fork the signatures file.

[uschindler]

@trejkaz interesting :). That'd be an argument for adding Date#toString to one of the plugin's blacklists. Though in most cases (as in your example) the invocation of #toString is implicit 🤔.

That is one problem that impossible to solve (very similar to the formatter problem). You cannot do that with static code analysis, because the runtime type is unknown at compile time (in most cases).

In Lucene we had the same problem: As we wanted to use BCP47 language tags everywhere we tried to forbid Locale#toString(), as Locale#toLanguageTag() should be used. Unfortunately this did not work as expected, as toString() is implicitely called by StringBuilders all the time (same for Java 9 indy string concats, where the toString() is called far inside the StringConcatFactory invokedynamic call).

So I have no idea how to fix the underlying issue. An "exclusion" is just a workaround!

[hakanai]

All those methods on Object are particularly troublesome. I figured it might be possible to detect the problem at runtime, but that doing it even at compile time was impossible.

invokedynamic adds a bunch of extra fun for APIs all over the place too. Now you pretty much have to ban methods just by name, right? :)

[uschindler]

See also #123 for an explanation why that's not possible to detect with a static byte code analysis tool. So I will close this as wont fix.