javaspotbugs

Why does this fix of a malicious code vulnerability (Spotbugs) work:

Ok, after I have got several comments about clarifying my equestion, I now will break down the question to the very essentials:

In my code there is a get-method for a variable (field) named "socket", which represents a correctly defined Java network socket of the type "Socket". This is the get method:

public Socket getSocket() {
 return socket;
}

The Spotbugs analysis of this code snippet results in a warning, which looks like this (cited letterwise, but lines separated by ">"):

> Malicious code vulnerability
> Method returning array may expose internal representation
> May expose internal representation by retruning reference to mutable object
> [package].[class].getSocket() may expose internal representation by returning [class].socket

If I modify the above code snippet to:

public Socket getSocket() {
 List<Socket> sockets = new ArrayList<>();
 sockets.add(socket);
 List<Socket> immutableSockets = Collections.unmodifiableList(sockets);
 return immutableSockets.get(0);
}

the Spotbugs warning vanishes.

I do not understand why, for the following reason: Spotbug analyzes the byte code, not the source code. So, if the latter code snipped just returns the reference to the mutable object likewise the first snippet, the byte code should be the same for both snippets. Up to now I have not concerned myself with details of Java compiler optimization algorithms but compilers usually optimize code snippets of such simple structure having the same result to the same machine level code (however, this may be a wrong perception). So, the latter "work around", in my eyes, should not have any effect on the Spotbugs warning, but it has, as noted above.

Now, from my point of view, there are three explanations right away:

  1. My understanding of Spotbugs is wrong and it analyzes not the byte code but at least the source code as well and, therefore, can be cheated by the second snippet as compared to the first one.
  2. This is a bug in Spotbugs.
  3. The second code snippet does not simply return the same reference to a mutable object, like the first one.

Finally I want to note, that this "work around" does work for any kind of reference to a mutable object, which is subject to a corresponding Spotbugs warning of the same kind as noted above, i.e., also for warnings leaded by "Storing reference to mutable object."

I do hope, that there is somebody out there to clarify this dubious situation and to give a satisfactory explanation for this behavior.

Finally let me note that, if the question is still not clear enough, it may be closed. This then means to me, that the observed behavior is not important enough to be considered more closely and I will not care either any more.

OK, I see that more clarification to my question is required. Let's try another approach:
First: the socket thing is only an example. The socket may be replaced by any arbitrary object, the Situation stays the same.
Second: Spotbugs analyzes the byte code, not the source code. Naming conventions and code structures, which do not alter anything should be irrelevant in my eyes, as the byte code still shows the problem of referencing a mutable object. So, both snippets should generate the same warning if both of them simply return a reference to a mutable object, the one directly and the other one as a list element. Why don't they do this? THIS is my question.

Solution

  • Your first code

    public Socket getSocket() {
    return socket;
}

    and the second code

    public Socket getSocket() {
    List<Socket> sockets = new ArrayList<>();
    sockets.add(socket);
    List<Socket> immutableSockets = Collections.unmodifiableList(sockets);
    return immutableSockets.get(0);
}

    are not resulting in the same byte code. Your statement

    So, if the latter code snipped just returns the reference to the mutable object likewise the first snippet, the byte code should be the same for both snippets.

    is simply not true. The first one only returns the value of a field, the second one involves different classes and method calls. It just happens to do kinda the same thing as in returning the field this.socket, only that the second code is doing it in a more "obfuscated" way. The Spotbugs library has only a limited set of patterns/detections, so it can't see that eventually the second code will leak the internal mutable object of your class.