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

Remove -auxclasspathFromInput to prevent System.in usage #535

Merged

Conversation

blacelle
Copy link

No description provided.

@blacelle
Copy link
Author

This refers to #488

@blacelle
Copy link
Author

Hello. Any feedback for this PR?

@hazendaz
Copy link
Member

hazendaz commented Feb 17, 2023 via email

@blacelle
Copy link
Author

blacelle commented Feb 17, 2023

@hazendaz This option is the one which in SpotBugs core triggers reading from System.in:

https://github.com/findbugsproject/findbugs/blob/master/findbugs/src/java/edu/umd/cs/findbugs/TextUICommandLine.java#L348-L361

java.lang.Thread.State: BLOCKED (on object monitor)
at java.io.BufferedInputStream.read(java.base@17.0.3/BufferedInputStream.java:334)
- waiting to lock <0x00000007000e5b60> (a java.io.BufferedInputStream)
at org.apache.tools.ant.Project.defaultInput(Project.java:1306)
at org.apache.tools.ant.Project.demuxInput(Project.java:1325)
at org.apache.tools.ant.DemuxInputStream.read(DemuxInputStream.java:73)
at sun.nio.cs.StreamDecoder.readBytes(java.base@17.0.3/StreamDecoder.java:270)
at sun.nio.cs.StreamDecoder.implRead(java.base@17.0.3/StreamDecoder.java:313)
at sun.nio.cs.StreamDecoder.read(java.base@17.0.3/StreamDecoder.java:188)
- locked <0x00000007010282b8> (a java.io.InputStreamReader)
at java.io.InputStreamReader.read(java.base@17.0.3/InputStreamReader.java:177)
at java.io.BufferedReader.fill(java.base@17.0.3/BufferedReader.java:162)
at java.io.BufferedReader.readLine(java.base@17.0.3/BufferedReader.java:329)
- locked <0x00000007010282b8> (a java.io.InputStreamReader)
at java.io.BufferedReader.readLine(java.base@17.0.3/BufferedReader.java:396)
at edu.umd.cs.findbugs.TextUICommandLine.handleOption(TextUICommandLine.java:411)

In the related core ticket, you said:

how maven plugin run would ever be wanting system input like that.

#488 (comment)

-> This option is used (but empty), hence it triggers reading from System.in, which lead to a dead-lock around System.in

@hazendaz
Copy link
Member

ok there are 3 different ones, isthink wrong was coded here.

Thinking something like this should have been coded.

        args << "-auxclasspath"
        args << "${project.compileClasspathElements}"

The GUI module would make more sense with asking input given its usage is interactive but even there it appears it was not properly setup for that either. It sets the path for GUI but doesn't use it. So I collected the item it would to set that to add here.

@hazendaz
Copy link
Member

after looking deeper and into original findbugs history, this was in the original. Further there were changes to fix invalid usage of aux including findbugs itself and finally, it was already properly set as expected shortly after this bit on call to spotbugs. I suspect this always caused thread issues but not seen outside case you mentioned. Given its been there for 11 years.

@hazendaz hazendaz merged commit 8f1f26a into spotbugs:spotbugs Feb 21, 2023
@blacelle blacelle deleted the RemoveUselessAuxclasspathFromInput branch February 21, 2023 05:42
@blacelle
Copy link
Author

@hazendaz Thanks for the validation. Do you have an ETA for next maven plugin release

@hazendaz
Copy link
Member

hazendaz commented Feb 21, 2023 via email

@pzygielo

This comment was marked as resolved.

@pzygielo

This comment was marked as outdated.

@hazendaz
Copy link
Member

@pzygielo Fix is in the works, doing some addition IT tests to make sure it doesn't get reintroduced as a problem in future. Expecting to have release out today.

@hazendaz hazendaz changed the title Remove -auxclasspathFromInput to prevent Systen.in usage Remove -auxclasspathFromInput to prevent System.in usage Jan 5, 2024
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

Successfully merging this pull request may close these issues.

None yet

3 participants