Skip to content

CPP: Workaround improvement for File.compiledAsMicrosoft.#1191

Merged
jbj merged 2 commits into
github:masterfrom
geoffw0:microsoft
Apr 5, 2019
Merged

CPP: Workaround improvement for File.compiledAsMicrosoft.#1191
jbj merged 2 commits into
github:masterfrom
geoffw0:microsoft

Conversation

@geoffw0
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 commented Apr 2, 2019

This will fix the 239 new results (in chakracore) on the internal CPP differences dashboard that showed up due to failing to correctly identify a Microsoft context. The fix is a heuristic, in the long run we need to make the extractor changes that were talked about a week or so ago.

Does not affect LGTM.

@geoffw0 geoffw0 added the C++ label Apr 2, 2019
@geoffw0 geoffw0 requested a review from a team as a code owner April 2, 2019 11:09
@jbj
Copy link
Copy Markdown
Contributor

jbj commented Apr 2, 2019

I know I'm the one who suggested this, but I feel bad about it already. As a workaround, it might do for now.

We should also support network paths like \\server\share\path\to\file and long-filename paths like \\?\C:\long\path. There are also funny names starting with \Device\LanmanRedirector and \Device\MUP (note: only one backslash), and we even had a customer once who compiled files from such paths. To support all those, it should suffice to check if the absolute path starts with \.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Apr 3, 2019

I know I'm the one who suggested this, but I feel bad about it already.

Yes, as you said, it's a workaround - and I'm hoping we can get extractor support to replace this in the not too distant future. @ian-semmle please could we turn this into a planned task if we haven't done so already?

To support all those, it should suffice to check if the absolute path starts with \

If you're happy that there are no Unix absolute paths that don't begin without a \, I'll switch to this strategy.

@jbj
Copy link
Copy Markdown
Contributor

jbj commented Apr 3, 2019

It turns out that File.getAbsolutePath converts \ to / on Windows, so a network share will be //server/share/path. It's documented in its QLDoc.

I think it'll definitely be a Windows file system if the second character is : (like you have now) or /. An absolute Unix path that's been normalised will have had duplicate slashes merged. To make sure this rules definitely won't match files compiled on Linux, I've started this query: https://lgtm.com/query/4684586825962173629/. The results aren't complete at the time of writing, but hopefully they will be soon.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Apr 4, 2019

I've started this query:

I actually ran a similar query with just the case for : previously. It looks like yours has no results so it should be safe to add the / case.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Apr 4, 2019

(done)

@jbj jbj merged commit 8c17278 into github:master Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants