You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by afine <gi...@git.apache.org> on 2017/04/18 19:51:06 UTC
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
GitHub user afine opened a pull request:
https://github.com/apache/zookeeper/pull/232
ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4: Malicious code vulnerability Warnings
There are two interesting parts to this change.
The first is in the Jute compiler. Fields that are declared buffers (translated to byte[] in java) now perform a clone in the constructor and while "getting and setting", following best practice. This prevents accidental changes to arrays once passed into or out of jute records but may negatively impact memory usage and performance. Would be interested in hearing if people think this is acceptable.
The second is in ZooDefs. We are currently declaring our predefined ACL lists with `new ArrayList<ACL>(Collections.singletonList(new ACL(...`. This seems strange to me as we appear to be converting a List type to an ArrayList. Would be great if someone could shed some light on why we do this. I think this logic can be simplified to `Collections.singletonList(new ACL(...` with the added bonus that the resulting list is immutable (making FindBugs happy).
Thanks,
Abe
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/afine/zookeeper ZOOKEEPER-2731
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/232.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #232
----
commit c5e3900bf768c6b4b1c0a2683be2b08259d328f8
Author: Abraham Fine <af...@apache.org>
Date: 2017-04-18T19:39:46Z
ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4: Malicious code vulnerability Warnings
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r117658709
--- Diff: src/java/main/org/apache/zookeeper/ZooDefs.java ---
@@ -96,21 +97,20 @@
/**
* This is a completely open ACL .
*/
- public final ArrayList<ACL> OPEN_ACL_UNSAFE = new ArrayList<ACL>(
- Collections.singletonList(new ACL(Perms.ALL, ANYONE_ID_UNSAFE)));
+ public final List<ACL> OPEN_ACL_UNSAFE =
--- End diff --
@afine , Its touching an exposed API and changing to List will impact b/w compatibility. Could you please follow the branch-3.5 fix like adding an exclude entry in `findbugsExcludeFile.xml`,
```
<!-- Disable 'Malicious code vulnerability warnings' due to mutable collection types in interface.
Undo this when ZOOKEEPER-1362 is done. -->
<Match>
<Class name="org.apache.zookeeper.ZooDefs$Ids"/>
<Bug pattern="MS_MUTABLE_COLLECTION" />
</Match>
```
API doc referenece http://zookeeper.apache.org/doc/r3.4.10/api/
Also, please read the discussions in https://issues.apache.org/jira/browse/ZOOKEEPER-1362
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r113011930
--- Diff: src/java/main/org/apache/jute/compiler/JType.java ---
@@ -27,7 +27,7 @@
private String mCName;
private String mCppName;
private String mCsharpName;
- private String mJavaName;
+ protected String mJavaName;
--- End diff --
Haven't looked into detail here, but one observation is that our trunk and branch 3.5 is find bug free, and it does not require this private - protected change here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #232: ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/232
@rakeshadr unrelated commit has been removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine closed the pull request at:
https://github.com/apache/zookeeper/pull/232
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #232: ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/232
@rakeshadr all your concerns should now be addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r112841400
--- Diff: src/java/main/org/apache/jute/compiler/JBuffer.java ---
@@ -41,6 +41,20 @@ public String genCppGetSet(String fname, int fIdx) {
public String getSignature() {
return "B";
}
+
+ public String genJavaGetSet(String fname, int fIdx) {
--- End diff --
Could you show me the findbug warning of `genJavaGetSet` .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r117358635
--- Diff: src/java/main/org/apache/jute/compiler/JType.java ---
@@ -27,7 +27,7 @@
private String mCName;
private String mCppName;
private String mCsharpName;
- private String mJavaName;
+ protected String mJavaName;
--- End diff --
@rakeshadr Apologies, I must not have been clear. There are no changes here that are for the "sake" of performance. In fact, my concern is that my changes will negatively impact performance.
After revisiting this change I think it makes more sense for us just to ignore the EI and EI2 issues. I reverted the changes to the jute compiler and added org.apache.zookeeper.server.quorum.QuorumAuthPacket to the findbugs exclusions file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r113032297
--- Diff: src/java/main/org/apache/jute/compiler/JType.java ---
@@ -27,7 +27,7 @@
private String mCName;
private String mCppName;
private String mCsharpName;
- private String mJavaName;
+ protected String mJavaName;
--- End diff --
@rakeshadr this fixes findbugs issues added by ZOOKEEPER-1045:
> Bug type EI_EXPOSE_REP
> In class org.apache.zookeeper.server.quorum.QuorumAuthPacket
> In method org.apache.zookeeper.server.quorum.QuorumAuthPacket.getToken()
> Field org.apache.zookeeper.server.quorum.QuorumAuthPacket.token
> At QuorumAuthPacket.java:[line 50]
and
> Bug type EI_EXPOSE_REP2
> In class org.apache.zookeeper.server.quorum.QuorumAuthPacket
> In method new org.apache.zookeeper.server.quorum.QuorumAuthPacket(long, int, byte[])
> Field org.apache.zookeeper.server.quorum.QuorumAuthPacket.token
> Local variable named token
> At QuorumAuthPacket.java:[line 35]
These issues are newer than the findbugs report included with the ZOOKEEPER-2728, which is why they are not listed there.
@hanm The reason that this solution is not used in 3.5 (and in other classes of 3.4) is because we ignore similar issues by including the following in findbugsExcludeFile.xml:
```
<Match>
<Package name="org.apache.jute.compiler.generated" />
</Match>
<Match>
<Package name="~org\.apache\.zookeeper\.(proto|data|txn)" />
<Bug code="EI, EI2" />
</Match>
<Match>
<Class name="org.apache.zookeeper.server.DataNode" />
<Bug code="EI2"/>
</Match>
<Match>
<Class name="org.apache.zookeeper.server.quorum.QuorumPacket" />
<Bug code="EI2, EI" />
</Match>
<Match>
<Class name="org.apache.zookeeper.ClientCnxn"/>
<Bug code="EI, EI2" />
</Match>
```
I went ahead and updated the patch to remove these entries and made some additional changes to get rid of all the findbugs warnings.
Although I am very concerned about the potential performance impact of including all of these extra clone() operations, particularly as it relates to "node data". What do you think, should we just ignore the warning on `QuorumAuthPacket.java` or fix the cause?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r117228583
--- Diff: src/java/main/org/apache/jute/compiler/JType.java ---
@@ -27,7 +27,7 @@
private String mCName;
private String mCppName;
private String mCsharpName;
- private String mJavaName;
+ protected String mJavaName;
--- End diff --
Thanks a lot @afine for pointing out the performance gains. I'd suggest to separate out the changes that helps to improve the performance from findbug fix because that would make the findbug fix/reviews simple. Also, iiuc perf related changes are applicable to all the branch codes and separate task would help us to track/merge the changes easily rather than clubbing multiple changes together in one commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #232: ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/232
Merged, please close pr @afine
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r117658774
--- Diff: src/java/main/org/apache/zookeeper/ZooDefs.java ---
@@ -96,21 +97,20 @@
/**
* This is a completely open ACL .
*/
- public final ArrayList<ACL> OPEN_ACL_UNSAFE = new ArrayList<ACL>(
- Collections.singletonList(new ACL(Perms.ALL, ANYONE_ID_UNSAFE)));
+ public final List<ACL> OPEN_ACL_UNSAFE =
+ Collections.singletonList(new ACL(Perms.ALL, ANYONE_ID_UNSAFE));
/**
* This ACL gives the creators authentication id's all permissions.
*/
- public final ArrayList<ACL> CREATOR_ALL_ACL = new ArrayList<ACL>(
- Collections.singletonList(new ACL(Perms.ALL, AUTH_IDS)));
+ public final List<ACL> CREATOR_ALL_ACL =
--- End diff --
Same as above, typically we won't modify the exposed APIs in 3.4.* releases. Its OK to exclude this case as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r112841388
--- Diff: src/java/main/org/apache/jute/compiler/JType.java ---
@@ -27,7 +27,7 @@
private String mCName;
private String mCppName;
private String mCsharpName;
- private String mJavaName;
+ protected String mJavaName;
--- End diff --
Could you show me the findbug warning of ` protected String mJavaName` statement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #232: ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/232
ZOOKEEPER-2722 commit is unrelated, please avoid that. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper pull request #232: ZOOKEEPER-2731: Cleanup findbug warnings in bra...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/232#discussion_r113032380
--- Diff: src/java/main/org/apache/jute/compiler/JBuffer.java ---
@@ -41,6 +41,20 @@ public String genCppGetSet(String fname, int fIdx) {
public String getSignature() {
return "B";
}
+
+ public String genJavaGetSet(String fname, int fIdx) {
--- End diff --
See https://github.com/apache/zookeeper/pull/232#discussion_r113032297
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] zookeeper issue #232: ZOOKEEPER-2731: Cleanup findbug warnings in branch-3.4...
Posted by rakeshadr <gi...@git.apache.org>.
Github user rakeshadr commented on the issue:
https://github.com/apache/zookeeper/pull/232
+1 LGTM, I'll commit this shortly.
Thanks @afine for the good work.
Thanks @hanm for the reviews.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---