debug/elf: calling `Open` on an empty file does not return a `&FormatError`

[MusicalNinjaDad]

Go version

go1.24.9 linux/amd64

Output of go env in your module/workspace:

AR='ar'
CC='gcc'
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_ENABLED='1'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
CXX='g++'
GCCGO='gccgo'
GO111MODULE=''
GOAMD64='v1'
GOARCH='amd64'
GOAUTH='netrc'
GOBIN=''
GOCACHE='/home/ninjacoder/.cache/go-build'
GOCACHEPROG=''
GODEBUG=''
GOENV='/home/ninjacoder/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFIPS140='off'
GOFLAGS=''
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2922389718=/tmp/go-build -gno-record-gcc-switches'
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMOD='/workspaces/snaggle/go.mod'
GOMODCACHE='/workspaces/snaggle/.go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/workspaces/snaggle/.go'
GOPRIVATE=''
GOPROXY='direct'
GOROOT='/usr/lib/golang'
GOSUMDB='off'
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/ninjacoder/.config/go/telemetry'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/lib/golang/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.24.9'
GOWORK=''
PKG_CONFIG='pkg-config'

What did you do?

shell: touch .empty

elffile, err = debug_elf.Open(".empty")

What did you see happen?

err is a naked io.EOF and not a &FormatError as would be expected. i.e.:

errors.Is(err, io.EOF) == true
var formaterr *debug_elf.FormatError
errors.As(err, &formaterr) == false

What did you expect to see?

As with other errors encountered by Open a &FormatError should be returned. Probably &FormatError{0, "no data"}

See https://github.com/MusicalNinjaDad/snaggle/commit/3c69325743767fcd2c8a69926fd02c99d2e92678#L172

[mknyszek]

In triage, we think that generally changing the type of an error returned is more iffy in terms of the backwards compatibility guarantee... but this is also a lesser used package and an odd corner case. Maybe it's fine.

[ianlancetaylor]

Currently the debug/elf package returns an error of type elf.FormatError for any error in the date of the ELF file. However, it does not return an error of that type for any error reading the ELF file. Errors reading the ELF file are returned unchanged. In particular, if the ELF file is too short, debug/elf will return io.EOF. This is true if the file is completely empty, and it is also true if the file is truncated at some point.

We certainly could change elf.Open and elf.NewFile to return elf.FormatError on any error reading from the file. But it's not obvious that we should do that.

I note that debug/macho and debug/pe act the same as debug/elf: they return FormatError for an error in the data, but return errors seen reading from the file unchanged.

I'm inclined to say that we shouldn't change anything here.

[MusicalNinjaDad]

Would you accept a simple documentation change to clarify this? The approach makes sense but isn't obvious - it's easy to assume that checking for a FormatError is sufficient to cover all cases.

I'd be happy to draft suitable wording for elf.Open, elf.NewFile and elf.FormatError

The alternative would be to wrap these cases in a fs.PathError which would be consistent with the rest of the stdlib but involve a chunk more work

[ianlancetaylor]

Documentation patches are always fine. Thanks.

At least the debug/macho package has documentation for its version of FormatError. The debug/elf and debug/macho docs should probably be identical or at least similar.

I see now that I was wrong about debug/pe. debug/pe defines FormatError but doesn't use it.

[MusicalNinjaDad]

After documenting the situation, I got puzzled as to why a raw io.EOF was returned, when os.Open usually returns a PathError.

A quick debug showed that the issue was actually from various Reader calls while parsing the ELF.

I have raised a second PR as a suggestion for handling these cases consistently and reporting the situation as a FormatError and ErrUnexpectedEOF which I believe accurately describes the semantics of the situation.

[gopherbot]

Change https://go.dev/cl/722980 mentions this issue: debug/elf: document errors returned by Open

[gopherbot]

Change https://go.dev/cl/723000 mentions this issue: debug/elf: return FormatError on UnexpectedEOF

[ianlancetaylor]

What is the benefit we gain for the increased complexity in the debug/elf code? Why is it useful to turn EOF or ErrUnexpectedEOF into FormatError?

I would find it somewhat easier to understand turning EOF into ErrUnexpectedEOF.

Also note that any change we make to debug/elf we should also make to debug/macho.

[MusicalNinjaDad]

Taking this into the debug/elf code removes the complexity from every single user of the library and puts it in one place where it can be done correctly & consistently. Leaving it out of the debug/elf code simply passes on the essential complexity to the much larger number of downstream users where it may easily be missed or incorrectly handled.

[ianlancetaylor]

Let me try to ask this in a different way. You say that this will remove complexity from every single user of debug/elf. But when I look at users of debug/elf, I do not see any complexity that would be removed at all. In general users of debug/elf check err != nil and report the error as appropriate for their use. They don't examine the error at all.

In general, being able to examine the contents of an error is only helpful if a program can take different actions based on the error details. In this case, what action could the program take? Knowing that the ELF file is invalid, for whatever reason, doesn't help to repair the ELF file, or to use parts of the ELF file but not other parts. What can a program do other than to report the error?

[MusicalNinjaDad]

I'll stick to the concrete usage that led to me noticing this and other use cases from the same codebase, rather than attempting to think of a long list of hypotheticals.

1. I have a command line app which can be invoked in the form snaggle DIRECTORY DESTINATION. In this case it will process every valid ELF in DIRECTORY (and skip any other files). I therefore loop through all the files calling the following function

   snagfile := func(path string) error {
   	var badelf *debug_elf.FormatError
   	err := snaggle(path, root, options, checker) // <- This calls elf.Open()
   	switch {
   	case err == nil:
   		return nil // snagged
   	case errors.As(err, &badelf):
   		return nil // not an ELF
   	default:
   		return err
   	}
   }
   

2. The same app can be invoked with snaggle FILE DIRECTORY. In this case I want my users to receive a meaningful error message if they attempt to provide an invalid FILE:

   foo@bar ~/$ snaggle .gitignore /tmp
   Error: parsing .gitignore:
   invalid ELF file: bad magic number '[35 33 47 117]' in record at byte 0x0    <- Useful
   
   foo@bar ~/$ snaggle .gitkeep /tmp
   Error: parsing .gitkeep:
   EOF    <- Not useful
   

   The first error is useful to the user - they know what the issue is, and if they were expecting the file to be a valid ELF they know where to look and what to look for. The second error is less useful to the user - while they know that an EOF was encountered they don't know where to look for that EOF, nor do they have a clear statement as to whether this is actually an issue with the input file, or potentially with the output file or something else in the inner workings of the app.

I do fully understand the trade-off between you taking on the maintenance responsibility vs. the value to end users. That's why I did not suggest, ask or expect you to go all the way to the far extreme and make FormatError expose the internal fields in the way that the canonical example of best-in-breed-stdlib errors fs.PathError does. That would help me with the following case, but would be almost certainly not worth the cost for you of cementing those fields into your API.

3. Not asking for this: I have a library which performs additional parsing on an ELF and relies on debug/elf for the underlying functionality. If FormatError were to be updated like PathError was and to provide FormatError.Off, FormatError.Err& FormatError.Data as part of the formal API, I would be able to avoid the need to create my own equivalent and simply repeat the pattern seen in os and which I've used elsewhere in my codebase, where a PathError made sense. That would allow me to provide any consumers of my library with a well-known and expected error API with a simple type FormatError = elf.FormatError declaration and then construct and return additional cases of FormatError.

[MusicalNinjaDad]

@ianlancetaylor - I guess I should close this and the associated PRs.

A clear "not interested in fixing error handling" up front would have been courteous and saved me spending a few hours of my personal time trying to help you with an improvement to your codebase in a way that prioritised removing any effort you would need to put in now or in the future.

[ianlancetaylor]

I'm sorry you feel frustrated. If we were definitely not interested, I would have said so. This was still on my list to think about more. Your last message was one week ago, and in that week we had the Go release freeze, which turned out to be unusually busy, and also the U.S. Thanksgiving holiday. I would like it if it were possible to respond promptly to every contribution and bug report, but that is not the world we live in. You should decide how you want to proceed, but if you do proceed some patience will be required.

[MusicalNinjaDad]

I clearly misinterpreted the recent communication. Thank you for the clarification and hope you enjoyed your Thanksgiving.

I'm happy to reopen this and leave the ball in your court. Please let me know if, when & how you would like to proceed and I'll more than happily (re-)raise a suitable PR. (Best to at-tag me as I'm not necessarily always on github checking silent notifications)

[randall77]

I'm ok with changing io.EOF to io.ErrUnexpectedEOF. Other parts of the stdlib do this, e.g. image/gif.

That said, I'm not sure that helps your use case all that much. The directory case behaves identically, and the individual file case would print "unexpected EOF" instead of "EOF", which I guess is somewhat better. But really the individual file case should be doing if errors.Is(err, io.EOF) and printing something better, in which case it doesn't really matter whether it is io.EOF or io.ErrUnexpectedEOF. (And changing the error return will break anyone who has a program which solved this problem by doing the former.)

[MusicalNinjaDad]

I'd agree that just changing io.EOF to io.ErrUnexpectedEOF would probably not be worth it e.g.:

	n, err := r.ReadAt(ident[0:], 0)
	if err != nil {
		if err == io.EOF {
			err = io.ErrUnexpectedEOF
		}
		return nil, err
	}

Would touch 10 places in the code, would be no benefit to anyone who is simply if err !=nil { return err } and would break anyone (including me) who has a workaround for current situation using if err == io.EOF || errors.As(err, &formatErr) { ... }. The sole advantage I can see would be that the returned error was technically correct and matched the documented usage from io(while EOF is technically an error it is semantically solely designed as a sentinel)

If the change is worth doing, then in my opinion, it only makes sense to change io.EOF to a &FormatError. This would touch the same 10 places in the code, but involve:

n, err := r.ReadAt(ident[0:], 0)
	if err != nil {
		if err == io.EOF {
			err = &FormatError{int64(n), io.ErrUnexpectedEOF.Error(), ident[0:n]}
		}
		return nil, err
	}

This would not break anyone with a workaround to the current situation (errors.As(err, &formatErr) would evaluate to true) and would avoid future users from needing to implement a workaround.

The question, I think, boils down to whether the change is worth doing at all, and how you consider the additional effort of maintaining the correct off & val if you ever need to adjust any of the returned errors.

Optionally, although with non-zero future maintenance effort adding an implementation of func (e *FormatError) Is(target error) bool would be a nice touch, but could raise expectations that are not worth the benefit today.