Resolve numerous high/medium Coverity findings #469

sjudeng · 2017-08-22T01:23:37Z

Also fixes some of the Fortify findings in #50 and #52 but not all the findings in those issues are fixed so the issues should remain open.

amcp

thanks, some comments

amcp · 2017-08-22T02:51:19Z

...sandra/src/main/java/org/janusgraph/diskstorage/cassandra/astyanax/AstyanaxStoreManager.java

-            for (ColumnFamilyDefinition cf : cluster.describeKeyspace(keySpaceName).getColumnFamilyList()) {
-                ks.truncateColumnFamily(new ColumnFamily<Object, Object>(cf.getName(), null, null));
+            final KeyspaceDefinition keyspaceDefinition = cluster.describeKeyspace(keySpaceName);
+            if (keyspaceDefinition != null) {


if (keyspaceDefinition == null) {
return;
}

amcp · 2017-08-22T02:51:31Z

...sandra/src/main/java/org/janusgraph/diskstorage/cassandra/astyanax/AstyanaxStoreManager.java

-                ks.truncateColumnFamily(new ColumnFamily<Object, Object>(cf.getName(), null, null));
+            final KeyspaceDefinition keyspaceDefinition = cluster.describeKeyspace(keySpaceName);
+            if (keyspaceDefinition != null) {
+                for (ColumnFamilyDefinition cf : keyspaceDefinition.getColumnFamilyList()) {


un-nest from if

amcp · 2017-08-22T02:55:25Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/database/StandardJanusGraph.java

@@ -669,6 +669,7 @@ public void commit(final Collection<InternalRelation> addedRelations,
            if (logTransaction) {
                //[FAILURE] Inability to log transaction fails the transaction by escalation since it's likely due to unavailability of primary
                //storage backend.
+                assert txLog != null;


why assert and not Preconditions.checkState? recall we/I was burned a long time ago regarding silent asserts in custom vertex id logic.

amcp · 2017-08-22T02:55:49Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/olap/computer/FulgoraGraphComputer.java

@@ -366,8 +366,10 @@ public void run() {
            try {
                for (Map.Entry<Long, Map<String, Object>> vprop : properties) {
                    Vertex v = tx.getVertex(vprop.getKey());
-                    for (Map.Entry<String, Object> prop : vprop.getValue().entrySet()) {
-                        v.property(VertexProperty.Cardinality.single, prop.getKey(), prop.getValue());
+                    if (v != null) {


if v == null continue
and then un-nest the below.

amcp · 2017-08-22T02:56:47Z

janusgraph-core/src/main/java/org/janusgraph/graphdb/relations/CacheEdge.java

@@ -46,7 +46,9 @@ public CacheEdge(long id, EdgeLabel label, InternalVertex start, InternalVertex
    }

    public Direction getVertexCentricDirection() {
-        return data.getCache().direction;
+        final RelationCache cache = data.getCache();
+        assert cache != null;


Preconditions.checkState(cache != null) ?

amcp · 2017-08-22T02:57:12Z

...ent/janusgraph-hadoop-core/src/main/java/org/janusgraph/hadoop/MapReduceIndexManagement.java

-                "";
+        final RelationType relationType =
+            RelationTypeIndex.class.isAssignableFrom(index.getClass()) ? ((RelationTypeIndex) index).getType() : null;
+        final String relationTypeName = relationType == null ? "" : relationType.name();


externalize the empty string?

amcp · 2017-08-22T02:57:22Z

...-hadoop-core/src/main/java/org/janusgraph/hadoop/config/job/AbstractDistCacheConfigurer.java

@@ -146,7 +146,7 @@ private static Conf configureByClasspath(String mapredJarFilename) {
        for (String cpentry : classpath.split(File.pathSeparator)) {
            if (cpentry.toLowerCase().endsWith(".jar") || cpentry.toLowerCase().endsWith(".properties")) {
                paths.add(new Path(cpentry));
-                if (cpentry.toLowerCase().endsWith(mrj));
+                if (cpentry.toLowerCase().endsWith(mrj))


amcp · 2017-08-22T03:03:07Z

@sjudeng also can you update the linked issues and convert the lists to checkboxes so we can track what is done and what is not done yet?

mbrukman · 2017-08-22T17:23:26Z

janusgraph-core/src/main/java/org/janusgraph/diskstorage/indexing/IndexFeatures.java

@@ -43,7 +43,7 @@ public IndexFeatures(boolean supportsDocumentTTL, Mapping defaultMap, ImmutableS
                         String wildcardField, ImmutableSet<Cardinality> supportedCardinaities, boolean supportsNanoseconds,
                         boolean supportCustomAnalyzer, boolean supportsGeoContains) {

-        Preconditions.checkArgument(defaultMap!=null || defaultMap!=Mapping.DEFAULT);


Why are we dropping the second check here? Was this meant to be && instead of ||, perhaps?

Thanks. I guess defaultMap==Mapping.DEFAULT wouldn't make sense. Incidentally I don't see any usages of the defaultMap (accessor is not covered according to codecov).

mbrukman · 2017-08-22T17:27:09Z

...-hadoop-core/src/main/java/org/janusgraph/hadoop/config/job/AbstractDistCacheConfigurer.java

@@ -146,7 +146,7 @@ private static Conf configureByClasspath(String mapredJarFilename) {
        for (String cpentry : classpath.split(File.pathSeparator)) {
            if (cpentry.toLowerCase().endsWith(".jar") || cpentry.toLowerCase().endsWith(".properties")) {
                paths.add(new Path(cpentry));
-                if (cpentry.toLowerCase().endsWith(mrj));
+                if (cpentry.toLowerCase().endsWith(mrj))


This is a behavior change — while it's most likely correct and the ; was a typo, it will now do assignments conditionally, whereas before, it would be unconditional.

Does this have potential to break some MapReduce workflows as a result? Should it be documented in release notes or otherwise that this is, while a correct change, is a potential issue to watch out for when upgrading from Titan to JanusGraph or from previous JanusGraph release to later ones?

@mbrukman Good point. But we might be okay. It definitely looks like a bug but more importantly it doesn't look like the actual JAR selected by this routine is relevant. The only usage originates from Hadoop2Compat where there's a comment that the actual JAR name (which corresponds to mapredJarFilename or mrj here) doesn't matter. I think if it did we would be seeing issues in Hadoop jobs since the incorrect job JAR is being used.

sjudeng · 2017-08-23T00:05:29Z

@amcp, @mbrukman - Updates are pushed. Thanks for the feedback.

@amcp I also updated the issues with current status. If you get a chance after this is merged it would be great if you could re-scan. Some of these were not indicated under Coverity.

pluradj

LGTM +1

sjudeng · 2017-09-14T01:57:16Z

@amcp, @mbrukman Anything else on this? Unless there are objections I'll plan to merge this in 48 hours.

mbrukman

Minor question.

mbrukman · 2017-09-14T02:09:06Z

janusgraph-core/src/main/java/org/janusgraph/util/system/ConfigurationLint.java

@@ -40,7 +41,7 @@ public static void main(String args[]) throws IOException {
            System.exit(1);
        }

-        log.info("Checking " + args[0]);
+        log.info("Checking configuration file");


Would it make sense to still print out the value of argv[0] here so that it's clear from the logs which file path is being validated? Otherwise, this log line might not be useful in isolation, when the user doesn't know the full path to the file that's being checked.

@mbrukman I updated to instead sanitize the input (and make a deep copy of the characters to clear the Fortify finding).

mbrukman

The PR looks good overall, thanks for fixing all of these issues! One question/comment below.

mbrukman · 2017-09-18T03:40:03Z

janusgraph-test/src/test/java/org/janusgraph/util/StringEncodingTest.java

-
+            } catch (IllegalArgumentException | NullPointerException e) { }
+
+            if (s != null) {


I understand that you're cleaning up Coverity findings here, but there's a cleaner way of writing tests that expect to fail, with a particular exception, e.g.,

https://stackoverflow.com/a/156528

https://stackoverflow.com/a/41031738

More details on both approaches (as well as historic workarounds): https://blog.goyello.com/2015/10/01/different-ways-of-testing-exceptions-in-java-and-junit/

My recommendation here would be to factor out the tests that are expected to fail into separate test methods, one for each invalid input (since each one fails in a different way). Since this is technically a Coverity cleanup PR, and not a test cleanup PR, it's up to you whether you want to fix it now, or add a TODO comment / file an issue to clean it up as a follow-up change.

@mbrukman Pushed fix to clean this up

…ify findings. Signed-off-by: sjudeng <sjudeng@users.noreply.github.com>

mbrukman

LGTM

vin0010 · 2018-08-27T07:19:09Z

janusgraph-core/src/main/java/org/janusgraph/core/JanusGraphFactory.java

@@ -313,10 +314,10 @@ private static final String getAbsolutePath(final File configParent, String file
        File storedir = new File(file);
        if (!storedir.isAbsolute()) {
            String newFile = configParent.getAbsolutePath() + File.separator + file;
-            log.debug("Overwrote relative path: was {}, now {}", file, newFile);
+            log.debug("Overwrote relative path: was {}, now {}", sanitizeAndLaunder(file), sanitizeAndLaunder(newFile));


Hi @sjudeng ,
I am trying to solve same problem in my project.
As fortify says "An attacker could take advantage of this behavior to forge log entries or inject malicious content into the log." and by looking at you code how com parsing each char of your string and build the string is going to help us prevent injecting?
Or is this method is just to suppress fortify scan?

Hi @vin0010. As you note there are two issues, one is mitigating the risk of log forging and the other is resolving the Fortify finding. Currently the sanitize implementation is minimal and just replaces newline characters, though this could definitely be extended to be more robust. The second part of the implementation recreates the string, which was still needed to clear the Fortify finding in this case even after sanitizing.

Resolve numerous high/medium Coverity findings

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Aug 22, 2017

amcp reviewed Aug 22, 2017

View reviewed changes

mbrukman reviewed Aug 22, 2017

View reviewed changes

sjudeng force-pushed the scan-fixes branch from ee53433 to f60c729 Compare August 22, 2017 23:49

This was referenced Aug 22, 2017

Fix log forging vunerabilities #50

Closed

Fix npe vunerabilities #52

Open

sjudeng force-pushed the scan-fixes branch from f60c729 to 9288b88 Compare August 23, 2017 23:33

pluradj approved these changes Aug 31, 2017

View reviewed changes

sjudeng force-pushed the scan-fixes branch from 9288b88 to b0d1db0 Compare September 4, 2017 14:37

mbrukman reviewed Sep 14, 2017

View reviewed changes

sjudeng force-pushed the scan-fixes branch 3 times, most recently from a45fe5b to b4f0f83 Compare September 16, 2017 13:31

mbrukman reviewed Sep 18, 2017

View reviewed changes

Resolve numerous high/medium Coverity findings. Resolve some NPE Fort…

8898e3b

…ify findings. Signed-off-by: sjudeng <sjudeng@users.noreply.github.com>

sjudeng force-pushed the scan-fixes branch from b4f0f83 to 8898e3b Compare September 18, 2017 09:45

mbrukman approved these changes Sep 18, 2017

View reviewed changes

sjudeng merged commit 92b85ee into JanusGraph:master Sep 18, 2017

vin0010 reviewed Aug 27, 2018

View reviewed changes

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#469 from sjudeng/scan-fixes

da87d78

Resolve numerous high/medium Coverity findings

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#469 from sjudeng/scan-fixes

135fbbe

Resolve numerous high/medium Coverity findings

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve numerous high/medium Coverity findings #469

Resolve numerous high/medium Coverity findings #469

sjudeng commented Aug 22, 2017

amcp left a comment

amcp Aug 22, 2017

amcp Aug 22, 2017

amcp Aug 22, 2017

amcp Aug 22, 2017

amcp Aug 22, 2017

amcp Aug 22, 2017

amcp Aug 22, 2017

amcp commented Aug 22, 2017

mbrukman Aug 22, 2017

sjudeng Aug 22, 2017 •

edited

mbrukman Aug 22, 2017

sjudeng Aug 22, 2017

sjudeng commented Aug 23, 2017

pluradj left a comment

sjudeng commented Sep 14, 2017

mbrukman left a comment

mbrukman Sep 14, 2017

sjudeng Sep 16, 2017

mbrukman left a comment

mbrukman Sep 18, 2017 •

edited

sjudeng Sep 18, 2017

mbrukman left a comment

vin0010 Aug 27, 2018

sjudeng Aug 27, 2018


		} catch (IllegalArgumentException \| NullPointerException e) { }

		if (s != null) {

Resolve numerous high/medium Coverity findings #469

Resolve numerous high/medium Coverity findings #469

Conversation

sjudeng commented Aug 22, 2017

amcp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcp commented Aug 22, 2017

Choose a reason for hiding this comment

sjudeng Aug 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudeng commented Aug 23, 2017

pluradj left a comment

Choose a reason for hiding this comment

sjudeng commented Sep 14, 2017

mbrukman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrukman left a comment

Choose a reason for hiding this comment

mbrukman Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrukman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudeng Aug 22, 2017 •

edited

mbrukman Sep 18, 2017 •

edited