-
Notifications
You must be signed in to change notification settings - Fork 17.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/vuln/cmd/govulncheck: include the source code location of init
#51575
Comments
cc @jba @zpavlinovic |
Sorry for not responding to this earlier, we are actually still thinking what to do about inits. We cannot get their location in most cases as they often do not actually appear in the source code.
I can see the trace has the location of where |
Reading the stack trace from the bottom to the top I think this is saying: package golang.org/x/build/internal/relui/db directly imports Hope that makes sense. @hyangah given the above explanation let me ask, what do you wish was printed? Once we know that we can figure out what metadata could provide this/is feasible. For reference, in the |
@timothy-king Using the import statement location of the problematic package seems like a reasonable choice. I used a similar heuristic (iterating over ASTs to find import statements) when converting such govulncheck traces into LSP diagnostics messages where position info is necessary. Some corner cases to think about though: in the following example,
When multiple files exist, arbitrarily choose one of them seems like a reasonable choice. @zpavlinovic What do you think about placing the location info next the calling function name's row? If the position information was displayed with the calling function names,
If the entry point is an |
If I understand correctly, the suggestion is to put the next to each call the location of the function being called rather than the location of the call itself? |
@zpavlinovic That's correct. I am not sure which one is better yet. When I click the link (some editors/terminals auto-linkify the file paths), I somehow expected it will land in a location (callsite of the next entry) inside the mentioned function (based on other crash or goroutine traces) |
Perhaps we should show both: the location of the function and the call itself. Something like this.
Call site location is useful because a function A can call function B at several different spots. Another useful thing here is that both static and dynamic callsites can be handled the same way (currently we use a different syntax for dynamically resolved callsites using -->) We could do away potentially with |
init
init
Note: the new version of govulncheck is now in x/vuln/cmd/govulncheck. The previous version is not supported anymore and has been deleted. |
In the new version of govulncheck,
When integrating govulncheck in linter or IDE, we still need to decide what location we can associate this info with. The natural place I can think of is the import & package statement. |
Change https://go.dev/cl/442499 mentions this issue: |
Every P.init function with no position is assigned the position of some "package P" statement. For P1.init calls P2.init case, position of the call is set to the position of some "import P2" statement in P1. For P.init calls P.init#d case, (P.init is implicit and) the position of the call to P.init#d is the position of some "package P" statement. Also refactors some code so that testing facilities can be reused and fixes some outdated documentation. Fixes golang/go#51575 Change-Id: Ib38dc396962353db8cc33e407ac7131a49d122c2 Reviewed-on: https://go-review.googlesource.com/c/vuln/+/442499 Reviewed-by: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
Every P.init function with no position is assigned the position of some "package P" statement. For P1.init calls P2.init case, position of the call is set to the position of some "import P2" statement in P1. For P.init calls P.init#d case, (P.init is implicit and) the position of the call to P.init#d is the position of some "package P" statement. Also refactors some code so that testing facilities can be reused and fixes some outdated documentation. Fixes golang/go#51575 Change-Id: Ib38dc396962353db8cc33e407ac7131a49d122c2 Reviewed-on: https://go-review.googlesource.com/c/vuln/+/442499 Reviewed-by: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
Every P.init function with no position is assigned the position of some "package P" statement. For P1.init calls P2.init case, position of the call is set to the position of some "import P2" statement in P1. For P.init calls P.init#d case, (P.init is implicit and) the position of the call to P.init#d is the position of some "package P" statement. Also refactors some code so that testing facilities can be reused and fixes some outdated documentation. Fixes golang/go#51575 Change-Id: Ib38dc396962353db8cc33e407ac7131a49d122c2 Reviewed-on: https://go-review.googlesource.com/c/vuln/+/442499 Reviewed-by: Jonathan Amsterdam <jba@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Zvonimir Pavlinovic <zpavlinovic@google.com>
In #51565
the govulncheck output is missing the exact source code location when
init
functions are involved.If the output presented the call site location of
x/text/language.MustParse
(inx/text/cases
),users could inspect how the problematic symbol is used more easily.
The text was updated successfully, but these errors were encountered: