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.

Sign up for GitHub

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

fix issue 26 #27

Closed
wants to merge 6 commits into from
Closed

fix issue 26 #27

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80a0229
fix issue 26
chavacava Jan 19, 2021
b841c18
clearer test implementation
chavacava Jan 21, 2021
0bdc91a
make it explicit here that we primarily intend this case for io.SeekEnd
chavacava Jan 22, 2021
11ea917
check err in the deferred seek to initial pos
chavacava Jan 23, 2021
3e00f49
Merge branch 'fix-26' of https://github.com/chavacava/kaitai_struct_g…
chavacava Jan 23, 2021
fc91f26
removes unnecessary trailing newline
chavacava Jan 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions kaitai/stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,17 @@ func (k *Stream) Size() (int64, error) {
if err != nil {
return 0, err
}
// Deferred Seek back to the current position
defer k.Seek(curPos, io.SeekStart)
Copy link
Member

@generalmimon generalmimon Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool https://github.com/golangci/golangci-lint noticed this problem:

$ ./golangci-lint-1.35.2-windows-amd64/golangci-lint run kaitai
kaitai\stream.go:67:14: Error return value of `k.Seek` is not checked (errcheck)
        defer k.Seek(curPos, io.SeekStart)
                    ^

This might potentially lead to a bug that this call fails, but because its error return value is ignored, k.Size() will happily return err == nil (despite the fact that the stream pointer failed to return to the original position for some reason and we're clearly in a situation of failure).

I think the code should be like this:

-func (k *Stream) Size() (int64, error) {
+func (k *Stream) Size() (int64, err error) {
Suggested change
defer k.Seek(curPos, io.SeekStart)
defer func() {
if _, serr := k.Seek(curPos, io.SeekStart); serr != nil {
err = serr
}
}()

See https://yourbasic.org/golang/defer/#use-func-to-return-a-value and https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1#afb5.

Could you please adjust the tests to check this case as well?

Perhaps we can make the failingReader more generic by restoring its normal behavior (i.e. io.SeekEnd behaving as normal, not setting pos to -1) and instead passing a param in it (e.g. numSeeksLimit) that limits the number of succeeding Seek() calls. Then we can just go iterate 0..(numSeeksThatSizeMethodDoes - 1) and check for each limit that the Size() function always returns an error and restores the stream position to its original state.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added two test cases:

  1. fails to get the initial pos
  2. the deferred seek fails


// Seek to the end of the File object
_, err = k.Seek(0, io.SeekEnd)
if err != nil {
return 0, err
}
// Remember position, which is equal to the full length
fullSize, err := k.Pos()
if err != nil {
return fullSize, err
}
// Seek back to the current position
_, err = k.Seek(curPos, io.SeekStart)
return fullSize, err

// Return the current position, which is equal to the full length
return k.Pos()
}

// Pos returns the current position of the stream.
Expand Down
38 changes: 38 additions & 0 deletions kaitai/stream_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package kaitai

import (
"bytes"
"errors"
"io"
"reflect"
"testing"
Expand Down Expand Up @@ -59,6 +60,7 @@ func TestStream_Size(t *testing.T) {
wantErr bool
}{
{"Size", NewStream(bytes.NewReader([]byte("test"))), 4, false},
{"Zero size", NewStream(bytes.NewReader([]byte{})), 0, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand All @@ -74,6 +76,42 @@ func TestStream_Size(t *testing.T) {
}
}

const initialPosition = 5
generalmimon marked this conversation as resolved.
Show resolved Hide resolved

type failingReader struct {
pos int64
}

func (fr *failingReader) Read(p []byte) (n int, err error) { return 0, nil }
func (fr *failingReader) Seek(offset int64, whence int) (int64, error) {
switch {
case whence == io.SeekCurrent && fr.pos != initialPosition:
return 0, errors.New("not allowed to seek to the current pos")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I fully understand this case. I guess that you're creating the error returned to the first k.Pos() called after k.Seek(0, io.SeekEnd), but this piece code is not exactly self-explanatory. It took me a while to figure out what do you want to achieve by the cryptic condition fr.pos != initialPosition .

And "not allowed to seek to the current pos" is not a very helpful message either (or let's call it a comment, because it's not printed anywhere). It made me think at first that instead of whence == io.SeekCurrent && fr.pos != initialPosition you must have meant whence == io.SeekCurrent && offset == 0 (so as not to allow seeking to the current fr.pos).

What about this:

	switch {
	case whence == io.SeekCurrent && fr.pos == -1:
		return 0, errors.New("artificial error when seeking with io.SeekCurrent after seeking to end")
	// ...
	default: // whence == io.SeekEnd
		fr.pos = -1
	}

It would help me if I saw the code for the first time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Done.

case whence == io.SeekCurrent:
return fr.pos, nil
case whence == io.SeekStart:
fr.pos = offset
default:
chavacava marked this conversation as resolved.
Show resolved Hide resolved
fr.pos++
}
return fr.pos, nil
}

// No regression test for issue #26
func TestErrorHandlingInStream_Size(t *testing.T) {
fr := &failingReader{initialPosition}
s := NewStream(fr)
_, err := s.Size()

if err == nil {
t.Fatal("Expected error, got nothing")
}
generalmimon marked this conversation as resolved.
Show resolved Hide resolved

chavacava marked this conversation as resolved.
Show resolved Hide resolved
if fr.pos != initialPosition {
t.Fatalf("Expected position to be %v, got %v", initialPosition, fr.pos)
}
}

func TestStream_Pos(t *testing.T) {
tests := []struct {
name string
Expand Down