Skip to content

Possible "index out of range" in equalMarks #97

Open

Description

@Antonboom

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/errors@v1.9.0/markers/markers.go:205
	github.com/cockroachdb/errors/markers.IsAny({0x102802528, 0x1400000e438}, {0x14000167f48, 0x1, 0x14000167f28?})
	        github.com/cockroachdb/errors@v1.9.0/markers/markers.go:186 +0x364
	github.com/cockroachdb/errors.IsAny(...)
	        github.com/cockroachdb/errors@v1.9.0/markers_api.go:64
	main.main()
	        examples/04-non-standard-modules/cockroach-is-any-bug/main.go:26 +0x318
	*/
}

Where am I wrong?

Activity

  1. knz commented on Jul 5, 2022

    @knz
    Contributor

    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.

  2. Antonboom commented on Jul 6, 2022

    @Antonboom
    Author

    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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions