{"action":"closed","issue":{"url":"https://api.github.com/repos/cdk/cdk/issues/523","repository_url":"https://api.github.com/repos/cdk/cdk","labels_url":"https://api.github.com/repos/cdk/cdk/issues/523/labels{/name}","comments_url":"https://api.github.com/repos/cdk/cdk/issues/523/comments","events_url":"https://api.github.com/repos/cdk/cdk/issues/523/events","html_url":"https://github.com/cdk/cdk/issues/523","id":395072278,"node_id":"MDU6SXNzdWUzOTUwNzIyNzg=","number":523,"title":"Various issues found by static analysis","user":{"login":"amaembo","id":5114450,"node_id":"MDQ6VXNlcjUxMTQ0NTA=","avatar_url":"https://avatars2.githubusercontent.com/u/5114450?v=4","gravatar_id":"","url":"https://api.github.com/users/amaembo","html_url":"https://github.com/amaembo","followers_url":"https://api.github.com/users/amaembo/followers","following_url":"https://api.github.com/users/amaembo/following{/other_user}","gists_url":"https://api.github.com/users/amaembo/gists{/gist_id}","starred_url":"https://api.github.com/users/amaembo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amaembo/subscriptions","organizations_url":"https://api.github.com/users/amaembo/orgs","repos_url":"https://api.github.com/users/amaembo/repos","events_url":"https://api.github.com/users/amaembo/events{/privacy}","received_events_url":"https://api.github.com/users/amaembo/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2019-01-01T10:36:32Z","updated_at":"2019-01-04T12:07:13Z","closed_at":"2019-01-04T12:07:12Z","author_association":"NONE","body":"Hello!\r\n\r\nI'd like to report a number of issues in CDK sources which were found by IntelliJ IDEA dataflow analysis. Here I mention only some important (to my opinion) issues which require attention. I don't report here redundant checks which are look harmless or potential inter-procedural nullability problems if I'm unsure that NPE could actually occur. I'm not going to submit a pull-request fixing these issues, because I'm not familiar with the code and may only speculate on how it should be properly fixed.\r\n\r\n1. org.openscience.cdk.renderer.generators.standard.AbbreviationLabel#norm\r\n\r\n```java\r\n        if (c > 128)\r\n            return 0;\r\n        switch (c) {\r\n            case '\\u002d': // hyphen\r\n            case '\\u2012': // figure dash << this branch is unreachable as c > 128 was excluded before\r\n            case '\\u2013': // en-dash << same \r\n            case '\\u2014': // em-dash << same\r\n            case '\\u2212': // minus << same\r\n                return '-'; // 002d\r\n            default:\r\n                return c;\r\n        }\r\n```\r\n\r\n2. org.openscience.cdk.qsar.descriptors.molecular.ALOGPDescriptor#getHAtomType (at the end of the method):\r\n```java\r\n        if (oxNum == 0) {\r\n            if (hybrid.equals(\"sp3\")) { ... \r\n            } else if (hybrid.equals(\"sp2\")) return 47;\r\n        } else if (oxNum == 1 && hybrid.equals(\"sp3\"))\r\n            return 47;\r\n        else if ((oxNum == 2 && hybrid.equals(\"sp3\")) || (oxNum == 1 && hybrid.equals(\"sp2\"))\r\n                || (oxNum == 0 && hybrid.equals(\"sp\"))) // << oxNum == 0 && hybrid.equals(\"sp\") is always false as oxNum == 0 was processed above. If this condition is true, 0 will be returned instead\r\n            return 48;\r\n        ...\r\n```\r\n\r\n3. org.openscience.cdk.geometry.AtomTools#calculate3DCoordinates0\r\n```java\r\n        Point3d points[] = new Point3d[0];\r\n        if (nwanted == 1) {\r\n            points = new Point3d[1];\r\n            points[0] = new Point3d(aPoint);\r\n            points[0].add(new Vector3d(length, 0.0, 0.0));\r\n        } else if (nwanted == 2) {\r\n            points[0] = new Point3d(aPoint); // << here points is zero-length array, so ArrayIndexOutOfBoundsException will occur. Probably points = new Point3d[2] is missing before this line\r\n            points[0].add(new Vector3d(length, 0.0, 0.0));\r\n            points[1] = new Point3d(aPoint);\r\n            points[1].add(new Vector3d(-length, 0.0, 0.0));\r\n```\r\nThe same problem in nwanted == 3 and nwanted == 4 branches, these branches will always fail with AIOOBE\r\n\r\n4. org.openscience.cdk.qsar.descriptors.bond.BondSigmaElectronegativityDescriptor#setParameters\r\n```java\r\n        if (!(params[0] instanceof Integer)) {\r\n            throw new CDKException(\"The parameter must be of type Integer\");\r\n        }\r\n        if (params.length == 0) return; // << cannot be true, because params[0] was checked before. If params is empty array, we would have AIOOBE thrown on the previous condition if(!(params[0] instanceof Integer))\r\n        maxIterations = (Integer) params[0];\r\n```\r\norg.openscience.cdk.qsar.descriptors.atomic.SigmaElectronegativityDescriptor#setParameters - same\r\n\r\n5. org.openscience.cdk.atomtype.CDKAtomTypeMatcher#perceiveBromine\r\n```java\r\n        } else if ((atom.getFormalCharge() != CDKConstants.UNSET && atom.getFormalCharge() == -1)) {\r\n            IAtomType type = getAtomType(\"Br.minus\");\r\n            if (isAcceptable(atom, atomContainer, type)) return type;\r\n        } else if (atom.getFormalCharge() == 1) { // << CDKConstants.UNSET is null. If it's really possible to see null here, then you will have NPE during auto-unboxing in this comparison. If it's impossible, then previous \"!= CDKConstants.UNSET\" check is redundant\r\n```\r\n\r\n6. org.openscience.cdk.smsd.filters.ChemicalFilters#getBondTypeMatches\r\n```java\r\nif (rStereo != 4 || pStereo != 4 || rStereo != 3 || pStereo != 3) { // << cannot be false as any number is either not 4 or not 3. Likely && was intended instead of ||\r\n```\r\n\r\n7. org.openscience.cdk.io.CIFReader#readChemFile (close to the method end)\r\n```java\r\nif (line != null) line = line.trim();\r\nwhile (!line.equals(\";\")) { // either null-check is redundant or (more likely) NPE is possible here\r\n```\r\n\r\n8. org.openscience.cdk.io.CrystClustReader#readChemFile (close to the method end)\r\n```java\r\nwhile (!line.startsWith(\"frame:\") && input.ready() && line != null) { // << line null-check after line.startsWith. Either null-check is redundant or NPE is possible here\r\n```\r\n9. org.openscience.cdk.math.IMatrix#IMatrix(org.openscience.cdk.math.Matrix)\r\n```java\r\n    public IMatrix(Matrix m) {\r\n        rows = m.rows;\r\n        columns = m.columns;\r\n        int i, j;\r\n        for (i = 0; i < rows; i++)\r\n            for (j = 0; j < columns; j++) {\r\n                realmatrix[i][j] = m.matrix[i][j]; // << this constructor does not allocate realmatrix and imagmatrix arrays, so NPE will occur when this constructor is used\r\n                imagmatrix[i][j] = 0d;\r\n            }\r\n    }\r\n```\r\n10. org.openscience.cdk.qsar.descriptors.atomic.IPAtomicHOSEDescriptor#extractInfo\r\n```java\r\n        int countSpace = 0;\r\n        boolean foundDigit = false;\r\n        for (int i = 0; i < strlen; i++) {\r\n            if (!foundDigit) if (Character.isLetter(str.charAt(i))) foundDigit = true;\r\n\r\n            if (foundDigit) {\r\n                if (Character.isWhitespace(str.charAt(i))) {\r\n                    if (countSpace == 0) { // << countSpace is never modified, so it's always 0 and condition is always true. Probably it's a mistake or unfinished code\r\n                        foundSpace = true;\r\n```\r\norg.openscience.cdk.qsar.descriptors.atomic.ProtonAffinityHOSEDescriptor#extractInfo - same problem\r\n\r\n11. org.openscience.cdk.smsd.Isomorphism#getEuclideanDistance\r\n```java\r\nif (getFirstMapping() != null || !getFirstMapping().isEmpty()) { // << the second part is executed only if getFirstMapping() returned null, in which case NPE is inevitable. Likely && was intended instead of ||\r\n```\r\n12. org.openscience.cdk.smsd.algorithm.mcgregor.McGregor#checkmArcs\r\n```java\r\nList<Integer> posNumList = new ArrayList<Integer>(size); // size does not set list size, it sets the initial size of backing array\r\n\r\nfor (int i = 0; i < posNumList.size(); i++) { // << this loop is never visited as posNumList is empty\r\n    posNumList.add(i, 0);\r\n}\r\n```\r\n\r\n13. org.openscience.cdk.io.formats.MDLFormat#matches\r\n```java\r\n        if (lineNumber == 4 && line.length() > 7 && (line.indexOf(\"2000\") == -1) && // MDL Mol V2000 format\r\n                (line.indexOf(\"3000\") == -1)) // MDL Mol V3000 format\r\n        {\r\n            // possibly a MDL mol file\r\n            try {\r\n                String atomCountString = line.substring(0, 3).trim();\r\n                String bondCountString = line.substring(3, 6).trim();\r\n                Integer.valueOf(atomCountString);\r\n                Integer.valueOf(bondCountString);\r\n                if (line.length() > 6) { // << always true as line.length() > 7 was already checked before; probably just redundant, but could be a typo\r\n```\r\n\r\n14. org.openscience.cdk.io.MDLV2000Writer#writeMolecule (around line 500)\r\n```java\r\n                while (last >= 0) {\r\n                    if (atomprops[last] != 0)\r\n                        break;\r\n                    last--;\r\n                }\r\n                // matches BIOVIA syntax\r\n                if (last >= 2 && last < atomprops.length) // << last < atomprops.length is always true as before exiting the previous loop we accessed atomprops[last]. Seems just redundant condition, but probably a mistake\r\n                    last = 5;\r\n```\r\n\r\n15. org.openscience.cdk.iupac.parser.MoleculeBuilder#addFunGroup (line 189)\r\n```java\r\naddAtom(\"Cl\", currentMolecule.isEmpty() ? null : currentMolecule.isEmpty() ? null : currentMolecule.getAtom(0), IBond.Order.SINGLE, 0); // << currentMolecule.isEmpty() is checked twice. Probably just redundant check, but something else could be intended\r\n```\r\n16. org.openscience.cdk.io.cml.PDBConvention#endElement\r\n```java\r\nif (currentMolecule instanceof IAtomContainer) { // << always true except currentMolecule == null case as currentMolecule declared type is IAtomContainer. Condition could be removed or replaced with a null check if null case is possible\r\n    logger.debug(\"Adding molecule to set\");\r\n    currentMoleculeSet.addAtomContainer(currentMolecule);\r\n    logger.debug(\"#mols in set: \" + currentMoleculeSet.getAtomContainerCount());\r\n} else if (currentMolecule instanceof ICrystal) { // << always false as ICrystal extends IAtomContainer, so this branch is never visited. Probably 'else' is redundant\r\n    logger.debug(\"Adding crystal to chemModel\");\r\n    currentChemModel.setCrystal((ICrystal) currentMolecule);\r\n    currentChemSequence.addChemModel(currentChemModel);\r\n}\r\n```\r\n17. org.openscience.cdk.io.RssWriter#write\r\n```java\r\nif (object instanceof IAtomContainer) {\r\n    root = convertor.cdkAtomContainerToCMLMolecule((IAtomContainer) object);\r\n} else if (object instanceof ICrystal) { // << similar to previous case: this branch is unreachable\r\n    root = convertor.cdkCrystalToCMLMolecule((ICrystal) object);\r\n```\r\n18. org.openscience.cdk.smsd.algorithm.vflib.VFlibMCSHandler#mcgregorFlag\r\n```java\r\nif (commonAtomCount > vfMCSSize && commonAtomCount > vfMCSSize) { // << repeated check, probably something else was intended\r\n```\r\norg.openscience.cdk.smsd.algorithm.vflib.VFlibSubStructureHandler#mcgregorFlag, org.openscience.cdk.smsd.algorithm.vflib.VFlibTurboHandler#mcgregorFlag - same\r\n19. org.openscience.cdk.smsd.algorithm.vflib.VFlibTurboHandler#setVFMappings\r\n```java\r\nInteger qIndex = Integer.valueOf(getReactantMol().indexOf(qAtom));\r\nInteger tIndex = Integer.valueOf(getProductMol().indexOf(tAtom));\r\nif (qIndex != null && tIndex != null) { // << condition is alays true: Integer.valueOf never returns null. Probably it was intended to check for -1 as -1 is returned by indexOf when atom not found\r\n    atomatomMapping.put(qAtom, tAtom);\r\n    indexindexMapping.put(qIndex, tIndex);\r\n} else {\r\n    try {\r\n        throw new CDKException(\"Atom index pointing to NULL\");\r\n    } catch (CDKException ex) {\r\n        LOGGER.error(Level.SEVERE, null, ex);\r\n    }\r\n}\r\n```\r\n"}}
{"actor":{"display_login":"egonw"}}