bazel-contrib / bazel-contrib/rules_go

go_path: embedsrcs path normalization uses lstrip() and can silently corrupt non-generated source paths

Open
#4,680 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
1.5k
Forks
760
Avg merge
1d 11h
Merged PRs (30d)
12

Description

`go_path` appears to mis-handle `embedsrcs` when source paths are plain source (not generated) because it uses `lstrip(ctx.bin_dir.path + "/")` as if that were exact prefix removal.

In `go/private/tools/path.bzl`:

```starlark
dst = pkg.dir + "/" + paths.relativize(
embedpath.lstrip(ctx.bin_dir.path + "/"),
src_dir.lstrip(ctx.bin_dir.path + "/"),
)
```

`lstrip` removes a *set of leading characters*, not a literal prefix.
So this can silently alter paths when they begin with characters that are present in `ctx.bin_dir.path + "/"`, producing incorrect relative paths without throwing an error.

## Why this is a bug

Using `lstrip(prefix)` is not equivalent to removing `prefix`:

- expected behavior: remove exact `ctx.bin_dir.path + "/"` prefix only when present
- actual behavior: repeatedly strip any leading chars in that character set

That can cause malformed `dst` for `embedsrcs`, especially for plain-source paths where these values aren’t both rooted under the same generated prefix.

## Why existing tests may miss this

`tests/core/go_path/go_path_test.go` (e.g. `TestEmbedPath`) may not trigger this because the fixture’s `embedsrcs` file and `.go` source share a directory prefix (like `example.com/repo/pkg/lib/...`).

With the current code, `lstrip(ctx.bin_dir.path + "/")` can corrupt both `embedpath` and `src_dir` in the same way; then `paths.relativize(...)` may effectively cancel that shared corruption.

The bug is more likely to surface when the `go_library` sources are at the module/package root (e.g. `src_dir == "."` or otherwise lacking a shared removable prefix), where there is no common prefix distortion to cancel out.

So a fix should include a **new targeted test case** for this shape, rather than relying on existing `go_path` embed tests.

## Suggested fix

Use exact-prefix removal semantics, e.g.:

- check `startswith(prefix)` then slice by `len(prefix)`, or
- equivalent helper that removes only the exact prefix.

## Additional context

I found this while investigating `go_path` + `embedsrcs` behavior; this seems distinct from existing issues about directory embedsrcs / generated embedsrcs placement.

Contributor guide

Open the contributing guide

Research direction

Start in go/private/tools/path.bzl, focusing on the embedsrcs path construction and how plain-source paths reach it. Review tests/core/go_path/go_path_test.go, especially TestEmbedPath, then add a targeted root-level source case and run the go_path tests. Done means non-generated embedsrcs paths retain their correct relative destinations without shared-prefix corruption.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
build-system
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.