Navigation Menu

Skip to content
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

Jump to bottom

x/text: potential Ineffective Assignments in a few packages #35136

Open
davewichers opened this issue Oct 24, 2019 · 3 comments
Open

x/text: potential Ineffective Assignments in a few packages #35136

davewichers opened this issue Oct 24, 2019 · 3 comments
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@davewichers
Copy link

Background: I'm a Software Security consultant, but not a Go expert. Just learning Go.

What version of Go are you using (go version)?

$ go version
 go version go1.13.1 darwin/amd64 (i.e., MacOS)

Does this issue reproduce with the latest release?

I believe I'm using the latest version of Go.

What did you do?

I ran a free Go tool called ineffassign (from: https://github.com/gordonklaus/ineffassign) and noticed it pointed out some issues in the go libraries themselves. I asked Gordon Klaus to review them and his response was: "Some of those are harmless (if sloppy), but others look like they might be real bugs. I think they're worth reporting at golang.org/issues."

What did you expect to see?

No issues pointed out in Go libraries by this tool.

What did you see instead?

I ran ineffassign on a Go app I'm doing a security review for and noticed these results as part of the output:

mpapp/vendor/golang.org/x/text/collate/build/colelem.go:205:5: ineffectual assignment to p
mpapp/vendor/golang.org/x/text/collate/maketables.go:449:4: ineffectual assignment to d
mpapp/vendor/golang.org/x/text/transform/transform.go:146:5: ineffectual assignment to err
mpapp/vendor/golang.org/x/text/unicode/bidi/core.go:489:5: ineffectual assignment to preceedingCharacterType
mpapp/vendor/golang.org/x/text/unicode/bidi/core.go:565:4: ineffectual assignment to i
mpapp/vendor/golang.org/x/text/unicode/bidi/core.go:644:4: ineffectual assignment to i
mpapp/vendor/golang.org/x/text/unicode/cldr/resolve.go:486:7: ineffectual assignment to err

These look like part of Go itself, so I find them interesting. Do you think these are real issues that need to be reported to the maintainers of Go? Or false positives? (Gordon said report them, so here they are.)
@bradfitz bradfitz changed the title Potential Ineffective Assignments in a few Go libraries x/text: potential Ineffective Assignments in a few packages Oct 24, 2019
@gopherbot gopherbot added this to the Unreleased milestone Oct 24, 2019
@bradfitz bradfitz added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 24, 2019
@bradfitz
Copy link
Contributor

/cc @mpvl

@filewalkwithme
Copy link
Contributor

Hi, if it's possible, I would like to work on this.

I had run ineffassign on x/text again an found more results. See the the updated list below.

Almost all issues can be easily fixed by simply removing or refactoring the ineffectual assignments clauses.

However, there are two entries that may actually need a deeper look:

  • ~/go/src/github.com/golang/text/unicode/bidi/core.go:575:4: ineffectual assignment to i
  • ~/go/src/github.com/golang/text/unicode/bidi/core.go:654:4: ineffectual assignment to i

These lines are part of a port of the reference implementation of the Bidi Parentheses Algorithm (found here: https://www.unicode.org/Public/PROGRAMS/BidiReferenceJava/BidiReference.java).

The original code uses basic FOR loops to iterate over an array, sometimes modifying the index inside the loop. At the other hand, the ported code from x/text uses a range FOR loop, and this may cause the algorithm to behave differently than expected. Example:

// Original Java Implementation:
for (int i = 0; i < length; ++i) {
    byte t = types[i];
    ...
    // skip over run of (former) neutrals
    i = runlimit;
}

// x/text port, using range instead basic loop:
for i, t := range s.types {
    ...
    // skip over run of (former) neutrals
    i = runlimit;   <------ INEFFECTUAL.
}

If it's ok, I can send a PR right away, fixing all issues.

Here is the updated list of entries generated by inefassign

~/go/src/github.com/golang/text/transform/transform.go:146:5: ineffectual assignment to err
~/go/src/github.com/golang/text/internal/number/decimal.go:381:3: ineffectual assignment to mult
~/go/src/github.com/golang/text/feature/plural/message.go:147:2: ineffectual assignment to form
~/go/src/github.com/golang/text/unicode/cldr/resolve.go:486:7: ineffectual assignment to err
~/go/src/github.com/golang/text/message/pipeline/pipeline.go:415:2: ineffectual assignment to args
~/go/src/github.com/golang/text/cmd/gotext/common.go:42:2: ineffectual assignment to args
~/go/src/github.com/golang/text/collate/build/colelem.go:205:5: ineffectual assignment to p
~/go/src/github.com/golang/text/unicode/bidi/core.go:499:5: ineffectual assignment to precedingCharacterType
~/go/src/github.com/golang/text/unicode/bidi/core.go:575:4: ineffectual assignment to i
~/go/src/github.com/golang/text/unicode/bidi/core.go:654:4: ineffectual assignment to i
~/go/src/github.com/golang/text/language/display/display.go:397:10: ineffectual assignment to reg
~/go/src/github.com/golang/text/encoding/encoding_test.go:56:3: ineffectual assignment to enc
~/go/src/github.com/golang/text/message/pipeline/pipeline_test.go:150:8: ineffectual assignment to err
~/go/src/github.com/golang/text/unicode/norm/normalize_test.go:943:2: ineffectual assignment to out

@davewichers
Copy link
Author

davewichers commented Mar 16, 2021 •
edited

@filewalkwithme - thanks for volunteering to work on this ..... 18 month old issue ...... :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @filewalkwithme @davewichers @gopherbot