code review 6936058: debug/elf: handle missing shstrndx in core files

debug/elf: handle missing shstrndx in core files

Fixes issue 4481.

hello-world-core.gz was generated with a simple hello world c program and core dumped as suggested in the issue. 

Also: add support for gz compressed test fixtures.

[dfc, 2012-12-16 01:11:55]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[minux1, 2012-12-16 01:22:33]

do we really need to create a tmp file?

i think just use bytes.Reader + bytes.Buffer and
elf.NewFile is acceptable for this case (elf.Open
is tested by others files and it merely open the
file and then invokes NewFile).

i'd like to avoid creating temporary files for this,
what do you think?

[dfc, 2012-12-16 01:33:53]

Done. PTAL.

[minux1, 2012-12-16 02:01:15]

https://codereview.appspot.com/6936058/diff/8001/src/pkg/debug/elf/file_test.go
File src/pkg/debug/elf/file_test.go (right):

https://codereview.appspot.com/6936058/diff/8001/src/pkg/debug/elf/file_test....
src/pkg/debug/elf/file_test.go:168: f, err := os.Open(tt.file)
well i mean we keep using elf.Open for normal files
so that we still test elf.Open and only use elf.NewFile
for gz compressed elf files.

[dfc, 2012-12-16 02:23:08]

> src/pkg/debug/elf/file_test.go:168: f, err := os.Open(tt.file)
> well i mean we keep using elf.Open for normal files
> so that we still test elf.Open and only use elf.NewFile
> for gz compressed elf files.

Gotcha. Done.

[dfc, 2012-12-16 02:25:05]

Hello golang-dev@googlegroups.com, minux.ma@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[minux1, 2012-12-16 02:32:02]

https://codereview.appspot.com/6936058/diff/4005/src/pkg/debug/elf/file.go
File src/pkg/debug/elf/file.go (right):

https://codereview.appspot.com/6936058/diff/4005/src/pkg/debug/elf/file.go#ne...
src/pkg/debug/elf/file.go:275: if shstrndx < 0 || shstrndx > shnum {
if shnum > 0 i think shstrndx should not be allowed to be
equal to shnum.

i think the correct condition is:
  if shstrndx < 0 || shstrndx != 0 && shstrndx >= shnum

[dfc, 2012-12-16 04:56:59]

https://codereview.appspot.com/6936058/diff/4005/src/pkg/debug/elf/file.go
File src/pkg/debug/elf/file.go (right):

https://codereview.appspot.com/6936058/diff/4005/src/pkg/debug/elf/file.go#ne...
src/pkg/debug/elf/file.go:275: if shstrndx < 0 || shstrndx > shnum {
On 2012/12/16 02:32:02, minux wrote:
> if shnum > 0 i think shstrndx should not be allowed to be
> equal to shnum.
> 
> i think the correct condition is:
>   if shstrndx < 0 || shstrndx != 0 && shstrndx >= shnum

PTAL. According to the freebsd man page on elf, 

"If a file has no section header table, both the e_shnum and the e_shoff fields
of the ELF 			header will be zero."

[rsc, 2012-12-17 14:51:32]

LGTM

[iant, 2012-12-17 20:50:36]

LGTM

https://codereview.appspot.com/6936058/diff/8006/src/pkg/debug/elf/file.go
File src/pkg/debug/elf/file.go (right):

https://codereview.appspot.com/6936058/diff/8006/src/pkg/debug/elf/file.go#ne...
src/pkg/debug/elf/file.go:276: if shnum > 0 && shoff > 0 && (shstrndx < 0 ||
shstrndx >= shnum) {
For the record, a value of shstrndx == SHN_XINDEX (0xffff) is valid and means
that the real shstrndx does not fit in 32 bits, and can be found in the sh_link
field of section header 0.  But that should be addressed separately at some
time.

[dfc, 2012-12-17 20:56:44]

> For the record, a value of shstrndx == SHN_XINDEX (0xffff) is valid and means
> that the real shstrndx does not fit in 32 bits, and can be found in the
sh_link
> field of section header 0.  But that should be addressed separately at some
> time.

Yup - there was a note about an extended segment header (probably not its name),
but you are probably right that we can cross that bridge when we come to it.

[dfc, 2012-12-17 20:59:27]

*** Submitted as https://code.google.com/p/go/source/detail?r=c2a84616b24e ***

debug/elf: handle missing shstrndx in core files

Fixes issue 4481.

hello-world-core.gz was generated with a simple hello world c program and core
dumped as suggested in the issue. 

Also: add support for gz compressed test fixtures.

R=minux.ma, rsc, iant
CC=golang-dev
https://codereview.appspot.com/6936058

[dvyukov, 2015-05-29 18:41:47]

https://codereview.appspot.com/6936058/diff/8009/src/pkg/debug/elf/file.go
File src/pkg/debug/elf/file.go (right):

https://codereview.appspot.com/6936058/diff/8009/src/pkg/debug/elf/file.go#ne...
src/pkg/debug/elf/file.go:276: if shnum > 0 && shoff > 0 && (shstrndx < 0 ||
shstrndx >= shnum) {
shouldn't this be:

	if shnum < 0 || shoff < 0 || shstrndx < 0 || shstrndx >= shnum {

?

If shnum is negative, we will crash later.
Negative shoff won't lead to an immediate crash, but still bogus I guess.

[rsc, 2015-05-29 20:15:32]

They are uint16s. They can't be negative.

[dvyukov, 2015-05-29 21:16:38]

On 2015/05/29 20:15:32, rsc wrote:
> They are uint16s. They can't be negative.

Then I don't understand how I am getting:

panic: runtime error: index out of range
goroutine 1 [running]:
debug/elf.NewFile(0x7fec629f7260, 0xc208014480, 0x645940, 0x0, 0x0)
	src/debug/elf/file.go:380 +0x1484

[iant, 2015-05-29 21:32:27]

On 2015/05/29 20:15:32, rsc wrote:
> They are uint16s. They can't be negative.

shnum is a uint16.  shoff is an int64 representing a value stored in ELF as
either uint32 or uint64; given a corrupt input file, it can be negative.

Note that this code doesn't handle valid ELF files with > 0xfff0 sections.

[dvyukov, 2015-05-30 10:08:12]

On 2015/05/29 21:32:27, iant wrote:
> On 2015/05/29 20:15:32, rsc wrote:
> > They are uint16s. They can't be negative.
> 
> shnum is a uint16.  shoff is an int64 representing a value stored in ELF as
> either uint32 or uint64; given a corrupt input file, it can be negative.
> 
> Note that this code doesn't handle valid ELF files with > 0xfff0 sections.

Filed https://github.com/golang/go/issues/10996 with standalone reproducer.
Let's move the discussion there.