Repository navigation
Rust: Update legacy MaD models 1 - #19934
geoffw0 merged 3 commits into
Conversation
Sorry, something went wrong.
geoffw0
left a comment
There was a problem hiding this comment.
There were quite a lot of changes in tests - mostly trivial changes to MaD model strings and references, but also a handful of lost sources...
Sorry, something went wrong.
| let path = e.path(); // $ MISSING: Alert[rust/summary/taint-sources] | ||
| let file_name = e.file_name(); // $ MISSING: Alert[rust/summary/taint-sources] | ||
| sink(path); // $ MISSING: hasTaintFlow | ||
| sink(file_name); // $ MISSING: hasTaintFlow |
There was a problem hiding this comment.
@hvitved I'm looking to get these results back. The sources go missing because there's no result for getCanonicalPath on lines 423 and 424, I'm guessing that means we haven't inferred the type of e / entry due to limitations of type inference on for loops (line 421).
If you agree that's the problem, do you have an idea what the general case for inferForLoopExprType should look like? I think we need to get the type of the Iterator (returned by fs::read_dir in this case) and pull it apart for it's Item type.
Sorry, something went wrong.
There was a problem hiding this comment.
You are right, it must be because we cannot currently infer the type of e, which means we cannot resolve the e.path() call (note that it is not e.path() itself that has a canonical path, it is the (missing) target that has a canonical path).
If you agree that's the problem, do you have an idea what the general case for inferForLoopExprType should look like? I think we need to get the type of the Iterator (returned by fs::read_dir in this case) and pull it apart for it's Item type.
Correct; I will give it a try.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks. Let me know if there's anything I can do to help.
Sorry, something went wrong.
There was a problem hiding this comment.
Here is a PR: #19971
Sorry, something went wrong.
|
DCA results - interestingly enough DCA does not show a loss of sources, it actually recognises a handful more, though a significant number of query sinks and 4 query results are lost. The lost results are the FPs we gained in #19881, I'm not too worried about them right now. The lost sinks I've narrowed down to the <core::option::Option>::expect model but I'm fairly sure that model is correct. However getCanonicalPath() is rarely succeeding in this cases, I think many are quite complex (generated in macros) and do not have a found getStaticTarget(). I don't think we should insist on fixing these now. I do think we need continue putting effort into getting type inference, call targets and canonical paths working in more cases to recover these losses. |
Sorry, something went wrong.
| let path = e.path(); // $ MISSING: Alert[rust/summary/taint-sources] | ||
| let file_name = e.file_name(); // $ MISSING: Alert[rust/summary/taint-sources] | ||
| sink(path); // $ MISSING: hasTaintFlow | ||
| sink(file_name); // $ MISSING: hasTaintFlow |
There was a problem hiding this comment.
You are right, it must be because we cannot currently infer the type of e, which means we cannot resolve the e.path() call (note that it is not e.path() itself that has a canonical path, it is the (missing) target that has a canonical path).
If you agree that's the problem, do you have an idea what the general case for inferForLoopExprType should look like? I think we need to get the type of the Iterator (returned by fs::read_dir in this case) and pull it apart for it's Item type.
Correct; I will give it a try.
Sorry, something went wrong.
Update some legacy MaD models to the new model format. This PR does perhaps a third of the models, I have more unfinished translations locally but I want to start pushing some of this work through CI + DCA and find out if there are going to be any issues there.