From 4bfb2b4138e70daed901b2bc3c1d71d60ea1e0cf Mon Sep 17 00:00:00 2001
From: Owen Mansel-Chan <owen-mc@github.com>
Date: Wed, 5 Aug 2020 16:24:24 +0100
Subject: [PATCH] Address review comments 1

---
 .../CWE-681/IncorrectIntegerConversion.qhelp  |  2 +-
 .../CWE-681/IncorrectIntegerConversion.ql     |  4 ++--
 ql/src/semmle/go/frameworks/Stdlib.qll        | 24 +++++++++----------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/ql/src/Security/CWE-681/IncorrectIntegerConversion.qhelp b/ql/src/Security/CWE-681/IncorrectIntegerConversion.qhelp
index 48682b46a7d..e0a39f59d82 100644
--- a/ql/src/Security/CWE-681/IncorrectIntegerConversion.qhelp
+++ b/ql/src/Security/CWE-681/IncorrectIntegerConversion.qhelp
@@ -25,7 +25,7 @@ the bit size you specified when parsing the number.
 </p>
 <p>
 If this is not possible, then add upper (and lower) bound checks specific to each type and
-bit size (you can find the minimum and maximum value for each type in the `math` package).
+bit size (you can find the minimum and maximum value for each type in the <code>math</code> package).
 </p>
 </recommendation>
 
diff --git a/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql b/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql
index c3049773d24..b27f290ed05 100644
--- a/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql
+++ b/ql/src/Security/CWE-681/IncorrectIntegerConversion.ql
@@ -64,7 +64,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
 
   override predicate isSource(DataFlow::Node source) {
     exists(ParserCall pc, int bitSize | source = pc.getResult(0) |
-      sourceIsSigned = pc.getTargetIsSigned() and
+      (if pc.targetIsSigned() then sourceIsSigned = true else sourceIsSigned = false) and
       (if pc.getTargetBitSize() = 0 then bitSize = 0 else bitSize = pc.getTargetBitSize()) and
       // `bitSize` could be any value between 0 and 64, but we can round
       // it up to the nearest size of an integer type without changing
@@ -74,7 +74,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
   }
 
   /**
-   * Holds if sink is a typecast to an integer type with size `bitSize` (where
+   * Holds if `sink` is a typecast to an integer type with size `bitSize` (where
    * 0 represents architecture-dependent) and the expression being typecast is
    * not also in a right-shift expression.
    */
diff --git a/ql/src/semmle/go/frameworks/Stdlib.qll b/ql/src/semmle/go/frameworks/Stdlib.qll
index f975cc13441..17ded52c27a 100644
--- a/ql/src/semmle/go/frameworks/Stdlib.qll
+++ b/ql/src/semmle/go/frameworks/Stdlib.qll
@@ -522,18 +522,18 @@ module StrConv {
     ParseFloat() { this.hasQualifiedName("strconv", "ParseFloat") }
   }
 
-  /** A function that parses integers with a specifiable bitSize. */
+  /** A function that parses integers with a specifiable bit size. */
   class ParseInt extends Function {
     ParseInt() { this.hasQualifiedName("strconv", "ParseInt") }
   }
 
-  /** A function that parses unsigned integers with a specifiable bitSize. */
+  /** A function that parses unsigned integers with a specifiable bit size. */
   class ParseUint extends Function {
     ParseUint() { this.hasQualifiedName("strconv", "ParseUint") }
   }
 
   /**
-   * A constant that gives the size in bits of an int or uint
+   * A constant that gives the size in bits of an `int` or `uint`
    * value on the current architecture (32 or 64).
    */
   class IntSize extends DeclaredConstant {
@@ -549,14 +549,14 @@ module ParserCall {
     abstract int getTargetBitSize();
 
     /** Holds if the type of the result number is signed. */
-    abstract boolean getTargetIsSigned();
+    abstract predicate targetIsSigned();
 
     /** Gets the name of the parser function. */
     abstract string getParserName();
   }
 }
 
-/** A call to a number-parsing function */
+/** A call to a number-parsing function. */
 class ParserCall extends DataFlow::CallNode {
   ParserCall::Range self;
 
@@ -566,7 +566,7 @@ class ParserCall extends DataFlow::CallNode {
   int getTargetBitSize() { result = self.getTargetBitSize() }
 
   /** Holds if the type of the result number is signed. */
-  boolean getTargetIsSigned() { result = self.getTargetIsSigned() }
+  predicate targetIsSigned() { self.targetIsSigned() }
 
   /** Gets the name of the parser function. */
   string getParserName() { result = self.getParserName() }
@@ -579,18 +579,18 @@ class ParserCall extends DataFlow::CallNode {
   }
 }
 
-/** A call to `strconv.Atoi` */
+/** A call to `strconv.Atoi`. */
 class AtoiCall extends DataFlow::CallNode, ParserCall::Range {
   AtoiCall() { exists(StrConv::Atoi atoi | this = atoi.getACall()) }
 
   override int getTargetBitSize() { result = 0 }
 
-  override boolean getTargetIsSigned() { result = true }
+  override predicate targetIsSigned() { any() }
 
   override string getParserName() { result = "strconv.Atoi" }
 }
 
-/** A call to `strconv.ParseInt` */
+/** A call to `strconv.ParseInt`. */
 class ParseIntCall extends DataFlow::CallNode, ParserCall::Range {
   ParseIntCall() { exists(StrConv::ParseInt parseInt | this = parseInt.getACall()) }
 
@@ -600,12 +600,12 @@ class ParseIntCall extends DataFlow::CallNode, ParserCall::Range {
     else result = this.getArgument(2).getIntValue()
   }
 
-  override boolean getTargetIsSigned() { result = true }
+  override predicate targetIsSigned() { any() }
 
   override string getParserName() { result = "strconv.ParseInt" }
 }
 
-/** A call to `strconv.ParseUint` */
+/** A call to `strconv.ParseUint`. */
 class ParseUintCall extends DataFlow::CallNode, ParserCall::Range {
   ParseUintCall() { exists(StrConv::ParseUint parseUint | this = parseUint.getACall()) }
 
@@ -615,7 +615,7 @@ class ParseUintCall extends DataFlow::CallNode, ParserCall::Range {
     else result = this.getArgument(2).getIntValue()
   }
 
-  override boolean getTargetIsSigned() { result = false }
+  override predicate targetIsSigned() { none() }
 
   override string getParserName() { result = "strconv.ParseUint" }
 }