SARIF has per-line rolling (partial) hash support #2605
Conversation
…ft/sarif-sdk into users/suvam/rolling-hash
(await) discussion with codeql dev
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
(await) discussion with codeql dev
…ft/sarif-sdk into users/suvam/rolling-hash
| using System; | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Sarif.Numeric |
There was a problem hiding this comment.
Could you delete this and just use C# long or ulong? The code does some funny things that look like it was ported from JavaScript.
There was a problem hiding this comment.
@KalleOlaviNiemitalo we did this port of the Long library to align the semantics with that of a Javascript implementation. The C# long was causing a behavior drift.
There was a problem hiding this comment.
This being a port of an Apache-2.0 licensed library, do you need to add third-party notices?
Do the rolling hashes need to be compatible with some other implementation, is the algorithm documented?
There was a problem hiding this comment.
The RollingHash code seems to be a port of https://github.com/github/codeql-action/blob/v2.1.39/src/fingerprints.ts.
What is the intended use of these hashes?
There was a problem hiding this comment.
Yes, exactly. CodeQL has shipped a very nice rolling hash that they've used historically for result matching. GHAS understands this fingerprint natively. Our goal here is to broaden the availability of this hash mechanism to other scenarios.
There was a problem hiding this comment.
@michaelcfanning Do we need to add any third-party notices as @KalleOlaviNiemitalo points out?
There was a problem hiding this comment.
Yes, see section 4 Redistribution of the apache license for this module. Let's get this in and immediately follow up with this.
suvamM
changed the title
suvamM
requested review from
EasyRhinoMSFT,
marmegh and
cfaucon
as code owners
src/Sarif/Numeric/Long.cs
Outdated
| private static readonly Dictionary<int, Long> INT_CACHE = new Dictionary<int, Long>(); | ||
| private static readonly Dictionary<uint, Long> UINT_CACHE = new Dictionary<uint, Long>(); |
There was a problem hiding this comment.
These dictionaries are not thread-safe. If they are mutated from multiple threads without locking, it can cause the dictionary to be corrupted such that every thread that subsequently tries to use it falls in an infinite loop.
At minimum, you need to use locks or switch to ConcurentDictionary or just a fixed-size array. But I'd really prefer it if Long were removed.
There was a problem hiding this comment.
@KalleOlaviNiemitalo Good catch. Switched to using ConcurrentDictionary. Unfortunately, as pointed out in a different comment, we need to use this port of Long to align the semantics with the CodeQL implementation. So the better option is to make this class thread safe.
There was a problem hiding this comment.
@KalleOlaviNiemitalo @michaelcfanning Could you kindly sign off on this change?
| } | ||
| } | ||
| } | ||
| catch (IOException) { } | ||
| catch (UnauthorizedAccessException) { } |
This PR adds a new hash utility to compute per-line rolling (partial) hashes for a given file. The PR also provides a port of a numeric library for representing and operating on 64-bit two's complement integers.