SonarQube says: “Non-serializable classes should not be written”
if (foo instanceof Serializable) {
s.writeObject(foo);
}
(s is some ObjectOutputStream)
In principle, sonarqube could know that foo is Serializable inside the body of the if.
I’ve seen Sonarqube follow impressive logic with variables that could be “null”. Compared to those findings, the recognizing of foo as “Serializable” in the sense of this rule appears almost feasible.
PS: in my real case, foo was really an indexed array foo[i] - just in case it matters.
It’s still SonarQube version 10.0.0.68432 - I know I should upgrade every few weeks, but I’m afraid, my customer doesn’t want me to spend my time mostly on doing upgrades…
I think the example is pretty clear in the context of “S2118”: Only “Serializeable” objects should be passed to writeObject, but Java itself doesn’t care - allows any Object. SQ tries to spot possible bugs and tells me when I pass a non-Serializeable-typed ref to the method, but it could notice the simple case of an “instanceof”-check guarding the call. That’s all.
Hi Andreas, thanks for the report.
I tried to reproduce with SQ 10.0.0.68432 but I had no luck.
Can I ask you to give me a code snippet complete of the class, methods and imports needed so that I can just copy and paste it into a new file and have it compile? That could help me reproduce your situation.
Also, how are you analyzing your file? Is it SonarLint connected to your local/remote SQ? Or are you using the Maven plugin?
A complete “SSCCE” is not an option here, but looking at the indicted method, I noticed that the objects to be written are stored in an array, and that this array is really an instance variable, not a local variable.
Maybe that makes my suggested change moot, as other threads might theoretically modify the instance array between the “instanceof”-check and its use for s.writeObject.
If that is the case, then I apologize.
I currently only have access to that one SonarQube installation at my customers’ and cannot make my own experiments on that, without them noticing, and they didn’t order a fix for that particular issue yet, and are also not likely to do it later… Will try to setup a local SQ here, so I can try my trim-downs in future before posting here.
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.io.IOException;
class Test implements Serializable {
protected transient Object[] parts = new Object[0];
/* skipped code to add an actual non-empty array of parts to the variable */
private synchronized void writeObject(ObjectOutputStream s) throws IOException {
s.defaultWriteObject();
for (int i = 0; i < parts.length; i++) {
if (parts[i] instanceof Serializable) {
s.writeObject(parts[i]);
}
}
}
}
regarding my use… On customer’s request we unpack our source tree (plus class files) on a customer’s machine, and there we let “sonar-scanner-4.8.0.2856-linux” scan it and it is configured to connect to sonarqube installaton on same machine.
(After that, they review the issues and order fixes for only some of them)
Hey there, thanks for the extra details.
Your snippet is still not reproducing the issue on my end, neither in SQ with the scanner analysis, nor in SL inside my IDE (Intellij).
From the way the check is implemented, it is not supposed to raise any issue at all when writing an Object in the output stream. Indeed I could also remove the guard check if (parts[i] instanceof Serializable) and the check would still not raise on my end. So that’s weird.
The only difference left between our environments I think is the fact that I am on Windows, but I don’t really think that would make a difference, I will anyway try to have a reproducer through a Linux scanner.
In the skipped code that you mentioned, is there anything relevant that we could add to this snippet that maybe could trigger the issue on my end?
One desperate attempt could be to add a setter for the local array variable.
Just let SQ think the array may have its value passed in from somewhere else…
Otoh, if this last example isn’t triggering the Rule for you, I wonder, if anything else
would… like parts = new Object[1] { new Object() }; (modulo typos)
or even more desperately: s.writeObject ( new Object() );
In my real code the array-type wasn’t exactly Object[] but an array of some (not-itself-Serializable-inheriting) Interface.
The actual instances, however are instances of Serializable implementations of that Interface.
Sorry that this is becoming tedious with me giving out details of the code one after another, but
it didn’t occur to me earlier, that SQ could have any reason to exclude Object specifically from that rule.
Hey Andreas, sorry for the late reply. Indeed that might have an effect on the issue being reported or not.
Could you share an updated self-contained and compiling snippet of code including this last condition you mentioned?
I still don’t have a local SonarQube, but here is another compileable example,
that is just another notch closer to the real code. I made sure it is compileable,
but if it still doesn’t trigger the issue, then I give up.
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.io.IOException;
interface Snafu {
void fubar();
}
class SnafuImpl implements Snafu, Serializable {
public void fubar() { }
}
public class Sonar_S2118 implements Serializable {
protected transient Snafu[] parts = new Snafu[] { new SnafuImpl() };
private synchronized void writeObject(ObjectOutputStream s) throws IOException {
s.defaultWriteObject();
for (int i = 0; i < parts.length; i++) {
if (parts[i] instanceof Serializable) {
s.writeObject(parts[i]);
}
}
}
}
Oh, wait… maybe we rather don’t initialize the array at all,
but add a setter for the array… That might get even closer,
opening the theoretical possibility of the array changing during
writeObjects’s execution… If that really is the key, then this
would mean, that this would also be the key to the issue being
a true positive…
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.io.IOException;
interface Snafu {
void fubar();
}
/*
class SnafuImpl implements Snafu, Serializable {
public void fubar() { }
}
*/
public class Sonar_S2118 implements Serializable {
protected transient Snafu[] parts;
public void setParts(Snafu[] parts) { this.parts = parts; }
public synchronized void writeObject(ObjectOutputStream s) throws IOException {
s.defaultWriteObject();
for (int i = 0; i < parts.length; i++) {
if (parts[i] instanceof Serializable) {
s.writeObject(parts[i]);
}
}
}
}
(latest edit: made writeObject “public”, just in case it also matters)
Thank you very much! Indeed this example reproduces the issue correctly. When the check looks at the instanceof expression, it fails to record as safe the type of the array. It can be easily fixed!
If it was the very last example that was reproducible - and not the but-last one - then there isn’t really anything to fix on sonarqube side…
In that case, it is correct to NOT accept the instanceof-check, because the array could be modified between the instanceof check and the re-retrieval of the array element for passing to the function…
If anything, then sonarqube could create the issue linked to a different (new?) rule, where it points out, that the guard isn’t safe.
The fix - on my side - would be to store the array element in a local variable (e.g. foo) and do it as I implied to have done it in my first post of the thread.
Oh, you are correct, I did not think about it when writing the ticket. Indeed the collections/arrays access was probably excluded in the first place for this exact reason.
What you suggested about a rule for non-safe guards is interesting though. I will dig into that and see what we can do!
