Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

False positive SING_SINGLETON_IMPLEMENTS_SERIALIZABLE with readResolve() present #2933

Open
rovarga opened this issue Apr 10, 2024 · 3 comments

Comments

@rovarga
Copy link
Contributor

rovarga commented Apr 10, 2024

A very basic singleton like this:

  public static final class Foo implements Serializable {
    private static final Foo INSTANCE = new Foo();

    private Foo() {
       // Hidden on purpose
    }

    private Object readResolve() {
        return VALUE;
    }
}

Triggers a SING_SINGLETON_IMPLEMENTS_SERIALIZABLE. While it is true a temporary instance is created during deserialization, that instance is always resolved back to INSTANCE, so only one instance is really observable.

In my case the offending code is here: https://github.com/opendaylight/yangtools/blob/master/common/yang-common/src/main/java/org/opendaylight/yangtools/yang/common/Empty.java and eliminating Serializable would break the overall design of the package.

@JuditKnoll
Copy link
Collaborator

Thank you for reporting this issue.

This seems to be intentional to me, see the readResolve() Method example's description at the MSC07 SEI Cert rule's site.

@rovarga
Copy link
Contributor Author

rovarga commented Apr 10, 2024

Ah, yes, after re-reading it couple of times, I see the attack vector now and have found one potential issue, the equals method relying on identity -- so that's cool, thanks! :)

The design intent of the above class is to have essentially a java.lang.Void, except on the other side of the nullness divide: whereas Voids cannot be anything but null, this behaves more like a normal value object would.

Anyway, it is not clear what are the criteria SpotBugs uses to identify the class as being a singleton -- as that is not the intent here. Perhaps the equals-based hints mentioned in #2934 would cover this as well.

@JuditKnoll
Copy link
Collaborator

If I understood correctly, even if the singleton class doesn't depend on the instance of the class at all, it should not be serializable (see Thomas Hawtin's comments at the MSC07 SEI Cert rule's site). Please, let me know, if I misunderstood something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants