Example: https://lgtm.com/projects/g/edemo/PDEngine/snapshot/0aef4231561da169109752d6fdb020f259db5f97/files/tools/getGithubIssues?sort=name&dir=ASC&mode=heatmap#x8b87d5afc0b67d28:1

Reported here: https://discuss.lgtm.com/t/false-positive-python-regexp/1967

It looks like character sets that include [ confuse the grouping. In the above regex, the only square brackets that define a character class sets are as follows (the rest are escaped or are in the class themselves):

\[(?P<txt>[^[]*)\]\((?P<uri>[^)]*)
          ^  ^              ^  ^

When the length of the array is confirmed to be 0 (mod n) where n is the increment value within the for loop, and this check is done immediately before the loop starts. As long as:

• the loop variable isn't modified anywhere else within the loop body
• the length of the array does not change (which will not happen for primitives unless reassigned).

we can be fairly certain that array accesses arr[i] .. arr[i + n - 1] will not be out-of bounds.

see: https://discuss.lgtm.com/t/false-positive-java-index-out-of-bounds-with-preconditions-check/1787

Alert:

https://lgtm.com/projects/g/cfengine/core/snapshot/4c539ec1d5521407a966fba34c93bb46d05d9895/files/libpromises/crypto.c?sort=name&dir=ASC&mode=heatmap#x35477c73e102d2eb:1

This function is used as a callback, passed as function pointer within the same file.

See: https://discuss.lgtm.com/t/java-false-positives/1787/2

Example: https://lgtm.com/projects/g/apache/accumulo/snapshot/dist-1984940991-1551192209962/files/core/src/main/java/org/apache/accumulo/core/file/rfile/bcfile/BCFile.java?sort=name&dir=ASC&mode=heatmap#xc3783b1655fd55ea:1

Final fields that are well-known, and for which we don't have the source code (e.g. Long.BYTES) are not considered ConstantIntegerExpr in RangeUtils.qll, so we incorrectly flag some multiplications as potential overflows.

Hi,

We just started using https://github.com/firehol/netdata in LGTM. Thank you!

We found that LGTM reports cpp/command-line-injection false positives.

Here is a screenshot:

But the code is the other way around: We use fgets() after we run the command, to read the output of the program we execute, like this:

https://lgtm.com/projects/g/firehol/netdata/snapshot/2a7cf3528a14cd50a69af4d75e1441a4b035d231/files/src/cgroup-network.c?#xb0514f82e375bcb6:1

Krita project fails to build because of a timeout, it can go to around 30% and then the systems kills the build.

https://lgtm.com/projects/g/KDE/krita/logs/languages/lang:cpp

False positive in TypeScript when base is a default export of another module, using export = syntax.

Alert

Forum post

cc: @styfle

As reported here: https://discuss.lgtm.com/t/false-positive-for-default-value-causing-early-return/1923

In this example: https://lgtm.com/projects/g/sympy/sympy/snapshot/190c7facbe879630c849b79962159f6b37459048/files/sympy/codegen/array_utils.py?sort=name&dir=ASC&mode=heatmap the mutation only occurs when a condition is met that is never true for the default value. As the default value is [], not [] will always be true when no parameter is passed in, so the default value will never me modified.

It seems that lgtm is showing a false positive for double checked locking:
https://lgtm.com/projects/g/micronaut-projects/micronaut-core/snapshot/4975e85649c04fa1bda488456cb1f26e19e01afd/files/inject/src/main/java/io/micronaut/context/DefaultBeanContext.java?sort=name&dir=ASC&mode=heatmap#x9ad3d6b82ea0bc8a:1

The code is using local variable to protect consistency as described in
https://wiki.sei.cmu.edu/confluence/display/java/LCK10-J.+Use+a+correct+form+of+the+double-checked+locking+idiom

last example - Compliant Solution (Immutable)

See: https://discuss.lgtm.com/t/java-false-positives/1787

Example: https://lgtm.com/projects/g/apache/accumulo/snapshot/dist-1984940991-1551192209962/files/core/src/main/java/org/apache/accumulo/core/clientImpl/ReplicationClient.java?sort=name&dir=ASC&mode=heatmap#xb063a21d9287922b:1

When the integer being multiplied is known to be small by range analysis (in this case, we can see that attempts is never greater than 10), then we should be able to determine that the multiplication does not overflow. It seems that this query currently takes into account when there is a constant value where we can work this out, but not when there is a maximum value.

Copied over from https://github.com/lgtmhq/lgtm-queries/issues/50

I don't see where a semicolon would be missing.

if (arg.startsWith('-'))
    throw new ConfigurationError(`Unknown option '${arg}'.`);

Hi,

I have a few alerts about js/xss I can't understand how to work around them.

Here is an example:

https://lgtm.com/projects/g/netdata/netdata/alerts/?mode=list&rule=js%2Fxss

As you can see, I have escaped everything, but still LGTM complaints about XSS:

function escapeUserInputHTML(s) {
    return s.toString()
        .replace(/&/g, '&')
        .replace(/</g, '&lt;')
        .replace(/>/g, '&gt;')
        .replace(/"/g, '&quot;')
        .replace(/#/g, '&#35;')
        .replace(/'/g, '&#39;')
        .replace(/\(/g,'&#40;')
        .replace(/\)/g,'&#41;')
        .replace(/\//g,'&#47;');
}

function verifyURL(s) {
    if(typeof(s) === 'string' && (s.startsWith('http://') || s.startsWith('https://')))
        return s
            .replace(/'/g, '%22')
            .replace(/"/g, '%27')
            .replace(/\)/g, '%28')
            .replace(/\(/g, '%29');

    console.log('invalid URL detected:');
    console.log(s);
    return 'javascript:alert("invalid url");';
}

Any ideas?

Thank you!

This is in regard to the "commented out code" query. If there is a curly brace inside a comment (/* advance to { */) this is misdetected as commented out code. It's for sure a border case, but given the fact that before the brace no valid C is given, I think it could be sorted out. It's also pretty hard to work around this type of FP.

Sample in context: https://lgtm.com/projects/g/rsyslog/rsyslog/alerts/?mode=list
Right now, you need to scroll down a bit to see the sample.

See:

• https://discuss.lgtm.com/t/c-false-positive-when-using-nested-range-for-loops/2174
• https://lgtm.com/projects/g/randombit/botan/snapshot/3132a920ebd0f523585d251820c29f6ed9e96aad/files/src/lib/tls/msg_server_hello.cpp?sort=name&dir=ASC&mode=heatmap#x307e5cb1eee8d376:1

__begin, __end and __range are incorrectly flagged as masked, even though the variables are not visible in the code. Perhaps we should always exclude these, or just exclude them when the variables are unused?

As reported here: https://discuss.lgtm.com/t/python-false-positive-the-result-of-coroutine-name-is-used-even-though-it-is-always-none/2135

It seems as though this usage for some reason does not detect that run_forever is async, and hence not a procedure.

cc @markshannon @taus-semmle

As per https://heycam.github.io/webidl/#dom-domexception-domexception (or MDN), a string name is acceptable as an optional second argument for DOMException, but we're getting this error...

https://lgtm.com/projects/g/dfahlander/typeson-registry/snapshot/839e0ea5856d5880007ae2b530f5065e1bd8aacf/files/presets/structured-cloning-throwing.js?sort=name&dir=ASC&mode=heatmap&showExcluded=false#xffaca8b7829dea90:1

As reported here: https://discuss.lgtm.com/t/possibly-incorrect-ql-python-cwe79/2186

It seems that we are not correctly identifying calls to make_response() as sinks for the xss query, even though it's used as an example in the query help.

Namely, lines 20 and 25 should be flagged but are not here: https://lgtm.com/projects/g/xia0AL/seven_flask/latest/files/hello.py?sort=name&dir=ASC&mode=heatmap#L20

js/react/unused-or-undefined-state-property doesn't take into account the static method getDerivedStateFromProps.

See example: https://lgtm.com/projects/g/ant-design/ant-design/snapshot/395c5ddd8d3db88e9c34cf332ec77b3c34077cfe/files/components/tag/index.tsx#xf4fd4ef83f04a4fd:1

The state property mounted is read in getDerivedStateFromProps() on line 48.

The documentation says that qmake is supported but such thing does not appears in the build output test.

https://lgtm.com/logs/dfa95438ea1c7edfcd9a7fae74ceda48e88972a4/lang:cpp

When exporting decorated functions.

See:

• https://lgtm.com/projects/g/rsokl/noggin/snapshot/215dff9c6c4c32e2fd0ac5f0411fcaf255c94e69/files/src/noggin/utils.py?sort=name&dir=ASC&mode=heatmap#x2b74f9ddfaa41951:1
• https://discuss.lgtm.com/t/python-false-positive-defined-function-is-labelled-undefined-because-it-is-decorated/2154

js/unused-local-variable has some false positives for variables that are intentionally unused, e.g. for array destructuring, etc...

An example can be seen in Shopify/quilt#434

It's a convention that local variables with leading underscores are designed to be unread, e.g:

const [a, _, c, d, e, f] = something
// or
const [a, _b, _c, d, e, f] = something

Moreover, tslint recently temporarily deprecated their no-unused-variable rule in favour of the new compiler flags which take leading underscores into account.

I suggest we:

• ignore variables that start with an underscore in js/unused-local-variable
• create a new query that finds variables that suggests they should be unused, but are actually used.

Per documentation here, glBegin takes 1 argument and glEnd takes none.

Link to forum post

Link to error on LGTM

Obj as parameter name is a supported parameter name when wrapped in an @click.group decorator.

See forum post issue

Link to error on LGTM

Hi

was not able to figure out how to build the QL and trying it locally

I am trying to extending QL for some more language semantics for JavaScript and JAVA

Can we please have such a note documented or instructions be shared

One of the links in the new GitHub Apps UI is broken:

https://github.com/cfengine/core/pull/3646/checks?check_run_id=117645973

The analysis for the merge commit was not successful. For more information, see the build and analysis logs on LGTM.com.

The other link, "View more details on LGTM.com" works.

Example:
https://lgtm.com/projects/g/dials/data/snapshot/5649de308532bce681716dc8235a851850c93bcd/files/dials_data/download.py#x29a04cee487f283d:1

In the snippet

try:
  import fcntl
except ImportError:
  pass

the except path is incorrectly identified as unreachable code. fcntl is a standard library module, but not present on Python for Windows.

In TypeScript it's valid to export an "ImportEqualsDeclaration".

export import stringify = require('./stringify');

// equivalent to
import stringify = require('./stringify');
export {stringify};

Currently this is reported as unused:
https://lgtm.com/projects/g/DefinitelyTyped/DefinitelyTyped/snapshot/d798d9208fc79327a67b16e60b73965d30f01ad7/files/types/iarna__toml/index.d.ts?sort=name&dir=ASC&mode=heatmap&showExcluded=false#x54f532a8afd9f620:1

See: https://discuss.lgtm.com/t/java-false-positives/1787/2

int x = ++i;
int y = ++i;
if (y < sb.length) {
  byte[] hex = {sb[x], sb[y]};
                ^^^^^ -- flagged as alert

We know that (modulo overflow):

• x = y - 1 --> x < y
• y < sb.length

Example: https://lgtm.com/projects/g/apache/accumulo/snapshot/b672a845ed56e9fd521ab85462cb6c45baa9abad/files/core/src/main/java/org/apache/accumulo/core/iterators/conf/ColumnSet.java?sort=name&dir=ASC&mode=heatmap#xb5a588dd1daff67e:1

The UnsafeUseOfStrcat.ql query reliably times out when run against the open source Pandas project when built on Linux, Python 3. I haven't tried running on Windows, so not sure if that's relevant.

[2018-10-18 15:29:16] [analysis] [EVALUATION 25/111] [FAIL] Error running query semmlecode-cpp-queries/Likely Bugs/Memory Management/UnsafeUseOfStrcat.ql: ExecutionFailed
                                 aborted query after 600014ms: the timeout was 600000ms.
[2018-10-18 15:29:16] [analysis]  Internal error during analysis for pandas - revision-2018-October-18--14-50-37:
                                 java.util.concurrent.CancellationException: Server shutdown
                                 com.semmle.queryserver.client.rpc.AbstractProtoBufClient.close(AbstractProtoBufClient.java:205)
                                 com.semmle.queryserver.client.CombinedQueryServerClient.close(CombinedQueryServerClient.java:204)
                                 com.semmle.cli.clientserver.client.SnapshotQueryRunner$Job.lambda$null$1(SnapshotQueryRunner.java:539)
                                 java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602)
                                 java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:577)
                                 java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:474)
                                 java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:1962)
                                 com.semmle.api.Java8EvaluationRun$1.acceptResult(Java8EvaluationRun.java:41)
                                 com.semmle.api.Java8EvaluationRun$1.acceptResult(Java8EvaluationRun.java:27)
                                 com.semmle.queryserver.client.TransformResultHandler.acceptResult(TransformResultHandler.java:24)
                                 com.semmle.queryserver.client.rpc.AbstractProtoBufClient$PairedResponseHandler.handleMessage(AbstractProtoBufClient.java:304)
                                 com.semmle.queryserver.client.rpc.AbstractProtoBufClient$ServerReadThread.run(AbstractProtoBufClient.java:402)

Per https://lgtm.com/projects/g/brettz9/webappfind/snapshot/3109d90b09b73d445f05fc17b71566bf38721aa7/files/executable-builder/executable.js?sort=name&dir=ASC&mode=heatmap&showExcluded=false#xddf739a1f6a2cb51:1 ,

..."Variable 'parentNode' always evaluates to false here."

However, this is not the case.

To illustrate with a simpler example, in the following code:

     var obj = {a: 5};
     var {a, b = a + 10} = obj;

...a from obj is destructured first to the local a, and it is then available to act as the default for b if b on obj is undefined (which in this case it is). As you can confirm in a modern JavaScript console, b will be 15 here because a is in fact available...

In my code, parentNode will not always evaluate to false because the local parentNode will be set based on the one present on target. In fact, it would, in my case, almost never be falsey...

See https://discuss.lgtm.com/t/python-false-positive-the-iter-method-of-iterable-class-xxx-does-not-return-an-iterator/2116

In the case where __iter__ returns an instance of a class that partially implements an iterator (e.g. implements __next__ but not __iter__), then the alert location and message can be confusing, as the problem actually lies with the definition of the iterator, and not the use of it.

As a suggested improvement, when returning an instance of a class that implements __next__ but not __iter__, the alert location should instead be the iterator class, and the message something like "This class is returned as an iterator {here}, but is missing a definition for __next__, so does not fully implement the iterator interface."

Despite my having an alert-suppression comment (at https://lgtm.com/projects/g/brettz9/C2D2/snapshot/4f8a73b02cc0ff4bc44c09a3ee6de709195d5712/files/explorercanvas/silverlight/excanvas.js?sort=name&dir=ASC&mode=heatmap&showExcluded=false#V158 ) and despite your site having my latest code, I find the alert is still showing. I guess this is because it is a multiline comment, but as your docs refer to the semi-colon, I was expecting it needed to be at the end of the statement...

Update: In the meantime, I just added a commit to add the alert suppression comment on the same line as the document.write, so will see what happens...

As detailed here: Shopify/quilt#434 (comment)

When doing object destructuring with new variable names, we're determining the name of the variable incorrectly, and selecting the name of the property from the object being destructured instead.

E.g:

const {foo: bar} = {foo: 'foo', bar: 'bar'}
// foo is undefined
// bar is now a constant variable

Example FP: https://lgtm.com/projects/g/Shopify/quilt/snapshot/b8540654494cf2f30293faccbaee3b4c19287e76/files/packages/react-google-analytics/src/GaJS.tsx#xf859505316cad831:1

The tool complains that qRegisterMetatype is a noop, it's not.

https://lgtm.com/projects/g/KDE/okular/snapshot/9755abc39706567915f1d1b757b70e2a0f8e3f3a/files/autotests/parttest.cpp?sort=name&dir=ASC&mode=heatmap#x9525a92bb944ee97:1

Following up on https://discuss.lgtm.com/t/false-positive-yield-import-results-in-syntax-error/1883/2 which frustratingly I am unable to post to. (I opened the issue, got a reply with a question, posted my reply to the question, but got told by the interface that my reply would not appear until it was approved by an owner or moderator or staff member or whatever, and then got another reply prodding me to please answer the question I already answered but which apparently will sit in a moderator queue for who-knows-how-long. And now the interface doesn't even offer the option of replying. Fortunately, the last reply included a link to this repo so here we are...)

My original report:

lgtm flags yield import blah as a Syntax Error https://lgtm.com/rules/1800086/ 2. Perhaps lgtm is using acorn-dynamic-imports prior to 2.0.2?

lgtm staff reply:

could you provide a link to the project in question please?

My reply which never got posted:

https://lgtm.com/projects/g/ilios/frontend/snapshot/dist-1507417986286-1552400119401/files/app/components/programyear-objective-list.js?sort=name&dir=ASC&mode=heatmap#xa9a0de30b91adc3e:1

Thanks!

This tracks old issue in lgtmhq/lgtm-queries: https://github.com/lgtmhq/lgtm-queries/issues/51

cc @ajafff @xiemaisi

The Java and JavaScript ql libraries have inconsistent naming for Expr member predicates that effectively do the same thing:

JavaScript: stripParens
https://github.com/Semmle/ql/blob/742c9708a9888c59391b49041ebb2f18ef2e1ffb/javascript/ql/src/semmle/javascript/Expr.qll#L83

Java: getProperExpr
https://github.com/Semmle/ql/blob/9012c3240f347655f859aa006817ff008b40aaf9/java/ql/src/semmle/code/java/Expr.qll#L50

I feel that we should unify the naming here, and deprecate whichever one we choose to no longer use, as this caused me some confusion when i was trying to find the Java version of this. IMO stripParens is a somewhat more obvious and intuitive name.

There appears to be an inconsistency for when py/non-iterable-in-for-loop is reported. It is reported for a list completion in the definition of an inner class, but not for the same list completion directly in the function body.

Alert

Forum Post

When a module is imported with import * as X, and X is used as a type definition in typescript, the import is incorrectly flagged as unused.

Example: https://lgtm.com/projects/g/synesthesia-project/server/snapshot/f005a51d7d89c42bfc64e785dcdc61fd567bf94f/files/src/main/connections/composer.ts?sort=name&dir=ASC&mode=heatmap#x77ff0d1aaf721abc:1

As reported here: https://discuss.lgtm.com/t/false-positive-resource-not-released-in-destructor/1938

https://lgtm.com/projects/g/KDE/kphotoalbum/snapshot/fe21e4ad620bf50c37ed9b9ec38a1941eb9b5fac/files/Exif/Database.cpp#x7903eb9a495cd189:1 2

In this case, the boolean member variable m_isOpen is mistaken for the resource that is acquired, when in fact the QSqlDatabase m_db holds the resource (and is correctly released in the dtor).

False positive of py/modification-of-default-value when a list is used as a default argument value, and then the argument is reassigned another value in the function.

Alert

Forum Post

In C++, one pattern for implementing operator+ and other overloaded binary operators is the following:

Type operator+(Type lhs, Type const& rhs ) {
   lhs += rhs;
   return lhs;
}

Exactly how this is compiled depends on both compiler version and optimization level, but it seems not to involve an additional copy at -O2 or -O3 on recent versions of the major compilers. See https://godbolt.org/z/W7BXKk for some experimentation with this.

Imported module is not available to pointsTo analysis, and b/c the query looks for attributes using pointsTo, it fails to find the variable.

Alert

Forum post

False positive in Python for logic surrounding nonlocal variables. Variable declared as nonlocal and used concurrently is being treated as an unused local variable, which leads to alerts here:

Alerts

Forum post.

CC: @michalc

In our code we have this:

    bool connected = false;
    bool should_retry = true;
    int remaining_tries = MAX_CONNECT_RETRIES;
    int ret;
    while (!connected && should_retry)
    {
        ret = SSL_connect(conn_info->ssl);
        ...
    }
    if (!connected)
    {
        TLSLogError(conn_info->ssl, LOG_LEVEL_ERR,
                    "Failed to establish TLS connection", ret);
        ...

and the use of ret is reported as potentially using an uninitialized variable [1]. Which I believe cannot happen because the condition in while() is always going to be true at least the first time and thus the variable ret will get initialized.

[1] https://lgtm.com/projects/g/cfengine/core/rev/pr-8b1d1654c2026687e81b86d5d27cd081a3060ab5#1320885514

Projects that use autobind should not display this error, as the methods are automatically bound in the constructor.

Link to error on LGTM

Link to autobind in constructor.

We noticed false positives when testing SAL annotations.

These three should demonstrate the issue:
sal.h:

#define _SAL_VERSION 20

test.cpp:

#include "sal.h"
int method1() {
    return _SAL_VERSION;
}
void method2();

test.ql:

import Microsoft.SAL
from SALAnnotation a
select a, a.getDeclaration()

Output from the query:

| test.cpp:3:12:3:23 | _SAL_VERSION | test.cpp:5:6:5:12 | method2 |

It seems the detection is based on "rank" and the fact that MacroInvocation _SAL_VERSION is before DeclarationEntry method2.

Not sure what is the best fix, can you please advise? Thank you.

While auditing some KDE software we got hit by a few false positives.
This one seems like one of them:

https://lgtm.com/projects/g/KDE/okular/snapshot/9755abc39706567915f1d1b757b70e2a0f8e3f3a/files/core/synctex/synctex_parser_utils.c?sort=name&dir=ASC&mode=heatmap#x6d7e052c9ef1e80:1

See: https://discuss.lgtm.com/t/java-false-positive-imho-cast-from-abstract-to-concrete-collection/2168

It seems that this query does attempt to filter when such a typecheck is present, but only takes into account raw types and not parameterized types with wildcards.

Example: https://lgtm.com/projects/g/fschopp/java-types/snapshot/6a6a2e9ec45512ab691927bcd767e300bf968389/files/src/main/java/net/florianschoppmann/java/reflect/ImmutableList.java#x1d138b35162e042f:1

Following a FP report, we see that even if the file path is checked for .., an alert is still reported. There should be an additional sanitizing guard such as !var.contains("..") in the query