You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by joshelser <gi...@git.apache.org> on 2017/07/26 15:58:28 UTC
[GitHub] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
GitHub user joshelser opened a pull request:
https://github.com/apache/accumulo/pull/283
ACCUMULO-4687 Clean up some static-analysis warnings
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/joshelser/accumulo 4687-fortify
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/283.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 #283
----
commit 2e66712008d185e23ac5bc1363afe4245ff3b2a8
Author: Josh Elser <el...@apache.org>
Date: 2017-07-26T15:57:40Z
ACCUMULO-4687 Clean up some static-analysis 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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129629727
--- Diff: core/src/main/java/org/apache/accumulo/core/util/Jar.java ---
@@ -41,8 +41,9 @@ public void execute(final String[] args) throws Exception {
String jarFileName = args[0];
String candidateMainClass = args.length > 1 ? args[1] : null;
Class<?> mainClass = null;
+ JarFile f = null;
try {
- JarFile f = new JarFile(jarFileName);
--- End diff --
+1 for try-with-resources
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129639517
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
Thanks, @ivakegg! Sounds like a good one to just pull the trigger on it and see if anything breaks ;)
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129616922
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
This is the one change which I'm not quite sure about.
It having been there since the dawn of time and without comment, I'm really not sure what it was intended to do. This predates all of the Kerberos work -- the only thing I can think of would have been the old SecurityManager constraints (but, if memory serves, those were more around SKVI's, not reading data from HDFS).
@billierinaldi, @keith-turner, @ericnewton, @scubafuchs: any guesses? :)
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129632689
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -264,4 +264,12 @@ protected static void banner(StringBuilder sb, String klass, String text) {
sb.append("<br />\n<h2 class='").append(klass).append("'>").append(text).append("</h2>\n");
}
+ /**
+ * Creates a {@link Cookie} with the given name and value, also setting the HttpOnly attribute on the cookie.
+ */
+ protected static Cookie createCookie(String name, String value) {
+ Cookie c = new Cookie(name, value);
+ c.setHttpOnly(true);
--- End diff --
The only cookie which actually needed to be protected was the session cookie addressed in ACCUMULO-4676. All of these cookies are simple user UI preferences (table column sort order, etc.) and it doesn't matter if they are manipulated in client-side javascript. Are we certain we're not manipulating them in client-side javascript now? If we are, this will break that.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by madrob <gi...@git.apache.org>.
Github user madrob commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129617491
--- Diff: core/src/main/java/org/apache/accumulo/core/util/Jar.java ---
@@ -41,8 +41,9 @@ public void execute(final String[] args) throws Exception {
String jarFileName = args[0];
String candidateMainClass = args.length > 1 ? args[1] : null;
Class<?> mainClass = null;
+ JarFile f = null;
try {
- JarFile f = new JarFile(jarFileName);
--- End diff --
Why not try-with-resources?
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129629434
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
I removed this in master branch with ACCUMULO-4498, and think it's safe to remove, but also think it could cause confusion for anybody who might have worked out a custom security policy for their environment. I think that's probably extremely low risk, though. I think it's fine to remove.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129976474
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -264,4 +264,12 @@ protected static void banner(StringBuilder sb, String klass, String text) {
sb.append("<br />\n<h2 class='").append(klass).append("'>").append(text).append("</h2>\n");
}
+ /**
+ * Creates a {@link Cookie} with the given name and value, also setting the HttpOnly attribute on the cookie.
+ */
+ protected static Cookie createCookie(String name, String value) {
+ Cookie c = new Cookie(name, value);
+ c.setHttpOnly(true);
--- End diff --
> I can double-check, but I'm not aware of that in 1.x. Not sure how the table sorting stuff works.
So I'm surprised, but this actually just doesn't break anything. Sorting on tables still works. Returning to a page after leaving retains the sort column+order.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by ivakegg <gi...@git.apache.org>.
Github user ivakegg commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129639159
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
I went back to the FSDataInputStream class all the way through 2006 and I cannot a version of the read(byte[], int, int) method which would require privledged access. This one is a mystery to me.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser closed the pull request at:
https://github.com/apache/accumulo/pull/283
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129632263
--- Diff: core/src/main/java/org/apache/accumulo/core/util/Jar.java ---
@@ -41,8 +41,9 @@ public void execute(final String[] args) throws Exception {
String jarFileName = args[0];
String candidateMainClass = args.length > 1 ? args[1] : null;
Class<?> mainClass = null;
+ JarFile f = null;
try {
- JarFile f = new JarFile(jarFileName);
--- End diff --
At a glance, I didn't think JarFile was Closeable, but that seems silly now.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129978175
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -264,4 +264,12 @@ protected static void banner(StringBuilder sb, String klass, String text) {
sb.append("<br />\n<h2 class='").append(klass).append("'>").append(text).append("</h2>\n");
}
+ /**
+ * Creates a {@link Cookie} with the given name and value, also setting the HttpOnly attribute on the cookie.
+ */
+ protected static Cookie createCookie(String name, String value) {
+ Cookie c = new Cookie(name, value);
+ c.setHttpOnly(true);
--- End diff --
We may only be using these server-side. Pretty sure we're refreshing the page and getting the resorted version from the server. I just wasn't sure if that was always the case, or if there was some fringe place where it would break.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129634763
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
Ahh, ok. Great. Not being able to attend that hackathon kept me in the dark, I guess :)
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129635623
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -264,4 +264,12 @@ protected static void banner(StringBuilder sb, String klass, String text) {
sb.append("<br />\n<h2 class='").append(klass).append("'>").append(text).append("</h2>\n");
}
+ /**
+ * Creates a {@link Cookie} with the given name and value, also setting the HttpOnly attribute on the cookie.
+ */
+ protected static Cookie createCookie(String name, String value) {
+ Cookie c = new Cookie(name, value);
+ c.setHttpOnly(true);
--- End diff --
> Are we certain we're not manipulating them in client-side javascript now? If we are, this will break that.
I can double-check, but I'm not aware of that in 1.x. Not sure how the table sorting stuff works. It would be nice to quell these warnings in the future, but I may begrudgingly have to revert.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129635806
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
I don't remember any discussion at the hackathon... I'm pretty sure it just happened to be where I was when I committed it. I don't think you missed anything.
---
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] accumulo pull request #283: ACCUMULO-4687 Clean up some static-analysis warn...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/283#discussion_r129639323
--- Diff: core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BoundedRangeFileInputStream.java ---
@@ -95,18 +92,7 @@ public int read(final byte[] b, final int off, int len) throws IOException {
throw new IOException("Stream closed");
}
in.seek(pos);
- try {
- ret = AccessController.doPrivileged(new PrivilegedExceptionAction<Integer>() {
--- End diff --
Oh, no no no, that's not what I meant. I only meant that I didn't recall seeing you make that change. It happening at the hackathon simply explains why I missed it.
---
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.
---