search

eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
157 stars 124 forks source link

classfmt: fix "Implicit narrowing conversion" warnings #2841

Closed jukzi closed 1 week ago

jukzi commented 3 weeks ago

ClassFileStruct.u4At(int) is supposed to return an unsigned integer. The return value is currently always used as primitive integer (signed) to compute readOffset inside the class file (which will then passed as argument to the method again). So any negative value would trigger an ArrayIndexOutOfBoundsException when used as index into the byte array.

The intermediate usage of primitive long did not help and only triggered warnings at the call sites.

https://github.com/eclipse-jdt/eclipse.jdt.core/security/code-scanning/18 ff

jukzi commented 3 weeks ago

https://github.com/eclipse-jdt/eclipse.jdt.core/security/code-scanning?query=pr%3A2841+tool%3ACodeQL+is%3Aopen show all targeted warnings gone

stephan-herrmann commented 3 weeks ago

I understand that carelessly using int will cause trouble for class files beyond 2 GiB. Apparently ecj hasn't yet produced or read a file of that size :)

But to really fix this, wouldn't we need to use long x for every this.reference[x] (of which there are plenty)? Am I missing anything?

IOW, you might be resolving some warnings, but I doubt that you are fixing a bug with your approach.

jukzi commented 3 weeks ago

Java arrays take only int as parameter. On top of that the java vm restricts array sizes to max int minus a hand full of bytes overhead. So jdt will never be able to read such a large class into an array. And i don't think there is any real world usecase for such. So this commit does not improve on that or fix any usecase but avoids needless cast to long and back to int. As a effect it will avoid the false positive warning.

stephan-herrmann commented 2 weeks ago

Java arrays take only int as parameter. On top of that the java vm restricts array sizes to max int minus a hand full of bytes overhead. So jdt will never be able to read such a large class into an array. And i don't think there is any real world usecase for such. So this commit does not improve on that or fix any usecase but avoids needless cast to long and back to int. As a effect it will avoid the false positive warning.

Thanks for explaining.

So in "any negative value would trigger an ArrayIndexOutOfBoundsException" you are discussing a purely hypothetical situation, right? I assume the situation would have blown up earlier as NegativeArraySizeException when trying to create that array.

jukzi commented 2 weeks ago

The exception may also depend on if the classfile is just too big (OOME) or broken / handcrafted with just illegal values inside. Such a thing might even cause infinite loops. This PR doesn't fix that.