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

Possible "index out of range" in equalMarks #97

Open
Antonboom opened this issue Jul 1, 2022 · 2 comments
Open

Possible "index out of range" in equalMarks #97

Antonboom opened this issue Jul 1, 2022 · 2 comments

Comments

@Antonboom
Copy link

Hello!

I'm interested in carefree iteration over lists whose lengths can vary:

// equalMarks compares two error markers.
func equalMarks(m1, m2 errorMark) bool {
	if m1.msg != m2.msg {
		return false
	}
	for i, t := range m1.types {
		if !t.Equals(m2.types[i]) {
			return false
		}
	}
	return true
}

And I made an example that breaks this code:

package main

import (
	"fmt"
	"github.com/cockroachdb/errors"
)

type SimpleWrapper struct {
	err error
}

func (w SimpleWrapper) Error() string {
	return "boom!"
}

func (w SimpleWrapper) Unwrap() error {
	return w.err
}

func main() {
	stack := errors.WithStack

	ref := stack(stack(SimpleWrapper{}))
	err := stack(stack(SimpleWrapper{err: stack(errors.New("boom!"))}))

	if errors.IsAny(err, ref) {
		fmt.Println("gotcha!")
	}

	/* panic: runtime error: index out of range [3] with length 3

	goroutine 1 [running]:
	github.com/cockroachdb/errors/markers.equalMarks(...)
	        github.com/cockroachdb/[email protected]/markers/markers.go:205
	github.com/cockroachdb/errors/markers.IsAny({0x102802528, 0x1400000e438}, {0x14000167f48, 0x1, 0x14000167f28?})
	        github.com/cockroachdb/[email protected]/markers/markers.go:186 +0x364
	github.com/cockroachdb/errors.IsAny(...)
	        github.com/cockroachdb/[email protected]/markers_api.go:64
	main.main()
	        examples/04-non-standard-modules/cockroach-is-any-bug/main.go:26 +0x318
	*/
}

Where am I wrong?

@knz
Copy link
Contributor

knz commented Jul 5, 2022

Hi, I believe part of the problem is that the ref error has an incomplete SimpleWrapper instance ( its err field is nil), which will mess up the type mark.
Does the error persist if you add a proper err to your SimpleWrapper reference?

In any case, I agree the ergonomics are not ideal, so perhaps there's something to improve here.

@Antonboom
Copy link
Author

Antonboom commented Jul 6, 2022

Does the error persist if you add a proper err to your SimpleWrapper reference?

No, now you won't break it quickly :)

You can close the issue, I don't mind, just expressed doubt.
Maybe add an explicit check for the lengths of ref lists?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants