search

spotbugs / spotbugs-maven-plugin

Maven Mojo Plug-In to generate reports based on the SpotBugs Analyzer
https://spotbugs.github.io/spotbugs-maven-plugin/
Apache License 2.0
69 stars 51 forks source link

Remove -auxclasspathFromInput to prevent System.in usage #535

Closed blacelle closed 1 year ago

blacelle commented 1 year ago

This refers to https://github.com/spotbugs/spotbugs-maven-plugin/issues/488

blacelle commented 1 year ago

Hello. Any feedback for this PR?

hazendaz commented 1 year ago

I do not believe that is related to issue you see. That isn't for console input. I haven't looked much more into it yet beyond its purpose but we don't want removed here.

Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Benoit Lacelle @.> Sent: Friday, February 17, 2023 6:48:27 AM To: spotbugs/spotbugs-maven-plugin @.> Cc: Subscribed @.***> Subject: Re: [spotbugs/spotbugs-maven-plugin] Remove -auxclasspathFromInput to prevent Systen.in usage (PR #535)

Hello. Any feedback for this PR?

— Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fspotbugs%2Fspotbugs-maven-plugin%2Fpull%2F535%23issuecomment-1434528960&data=05%7C01%7C%7Cad1abc609347436cfee808db10dce1f6%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638122313082892030%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=BZ7gsg7TT%2BEvtevTWdmpglnhXJYG6Z3cxeA03iWKruU%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAHODI6DVNXKDJXRSLSDRP3WX5QQXANCNFSM6AAAAAAUIMNMPQ&data=05%7C01%7C%7Cad1abc609347436cfee808db10dce1f6%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638122313082892030%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=3Rrds52hJaNyEX5kKLqj50zNM0Pgv%2BYluxsyVRvG%2BtA%3D&reserved=0. You are receiving this because you are subscribed to this thread.Message ID: @.***>

blacelle commented 1 year ago

@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.

https://github.com/spotbugs/spotbugs-maven-plugin/issues/488#issuecomment-1404312625

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

hazendaz commented 1 year ago

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 commented 1 year ago

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.

blacelle commented 1 year ago

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

hazendaz commented 1 year ago

Next weekend is my plan but that could go over. I'm trying to resolve two issues with the current open pull requests before I release.

Sent from my Verizon, Samsung Galaxy smartphone Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Benoit Lacelle @.> Sent: Tuesday, February 21, 2023 12:43:49 AM To: spotbugs/spotbugs-maven-plugin @.> Cc: Jeremy Landis @.>; Mention @.> Subject: Re: [spotbugs/spotbugs-maven-plugin] Remove -auxclasspathFromInput to prevent Systen.in usage (PR #535)

@hazendazhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fhazendaz&data=05%7C01%7C%7C51e78743dadf49ddad2e08db13ce9c33%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638125550328938663%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uKC%2Fnrwq6oJ9MjUw1snRSBriFb%2Bj4uKAihSQAn%2FZK4A%3D&reserved=0 Thanks for the validation. Do you have an ETA for next maven plugin release

— Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fspotbugs%2Fspotbugs-maven-plugin%2Fpull%2F535%23issuecomment-1437891428&data=05%7C01%7C%7C51e78743dadf49ddad2e08db13ce9c33%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638125550328938663%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=d1ENd67m6%2Bzy9zzTT9F%2FSleh5Y%2FPS0bJeO1R8bM1FYQ%3D&reserved=0, or unsubscribehttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAAHODI6VN6AN23GRPYUEFL3WYRIZLANCNFSM6AAAAAAUIMNMPQ&data=05%7C01%7C%7C51e78743dadf49ddad2e08db13ce9c33%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638125550329094903%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=S3OPfmuGGYJxBuZ%2BImw7%2BGGYK8g63h%2Ft4IQ3oJyKaeI%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>

pzygielo commented 1 year ago

I might be wrong, but for now it seems to me that this broke my build.

Reproduced here: https://github.com/pzrep/spotbugs-SE_TRANSIENT_FIELD_OF_NONSERIALIZABLE_CLASS/actions/runs/4270236377/jobs/7433909356#step:4:305

first bad commit: [e6cb4751ac067a9232442784e7bd9a2e86d0b130] Remove -auxclasspathFromInput to prevent Systen.in usage
pzygielo commented 1 year ago

Ok, I see other reports

raised already.

hazendaz commented 1 year ago

@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.