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

Fix: virtualFile.Read returns EOF with valid data #159

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cmaglie
Copy link

@cmaglie cmaglie commented Jul 6, 2020

It happens that the read method in VirtualFile may return EOF together with data.
This is not how the os.File behaves normally as explained in https://golang.org/pkg/os/#File.Read

func (f *File) Read(b []byte) (n int, err error)

Read reads up to len(b) bytes from the File. It returns the number
of bytes read and any error encountered. At end of file, Read
returns 0, io.EOF.

This bug leads to an infinite loop when I tried to read openpgp keyring using the golang builting openpgp package.
After some digging I found that the library relies on the fact that Read sends the EOF alone (without valid data).

This pull request fix the wrong behaviour.

I've added also a test to check how the os.File.Read behaves compared to rice.File.Read.

=== RUN   TestVirtualFileRead
    TestVirtualFileRead: virtual_test.go:33: 
                Error Trace:    virtual_test.go:33
                                                        virtual_test.go:37
                Error:          Not equal: 
                                expected: "n=4 err=<nil> buff=[84 69 83 84]"
                                actual  : "n=4 err=EOF buff=[84 69 83 84]"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -n=4 err=<nil> buff=[84 69 83 84]
                                +n=4 err=EOF buff=[84 69 83 84]
                Test:           TestVirtualFileRead
--- FAIL: TestVirtualFileRead (0.00s)
FAIL
exit status 1
FAIL    github.com/GeertJohan/go.rice   0.004s

I would like to also thank you for go.rice, we are using it successfully in the arduino-cli.

It happens that the read method in VirtualFile may return EOF together
with data. This is not how the os.File behaves normally as explained
in https://golang.org/pkg/os/#File.Read

> func (f *File) Read(b []byte) (n int, err error)
>
> Read reads up to len(b) bytes from the File. It returns the number
> of bytes read and any error encountered. At end of file, Read
> returns 0, io.EOF.
cmaglie added a commit to cmaglie/arduino-cli that referenced this pull request Jul 6, 2020
This version must be kept until the following pull request is merged
upstream: GeertJohan/go.rice#159
cmaglie added a commit to arduino/arduino-cli that referenced this pull request Jul 7, 2020
* Added download of signature files for 'downloads.arduino.cc' domain

* Slightly simplified tmp file creation

* Use patched version of rice-box

This version must be kept until the following pull request is merged
upstream: GeertJohan/go.rice#159

* Added signature verification subroutines

* Implemented package_index.json signature verification

* Added missing license headers

* Added negative test on signature check

* Only copy signature if the file is present
Copy link

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

Change looks good to me. I've also tested this version, and it works as expected along with openpgp (which is the same case @cmaglie ran into).

cmaglie added a commit to cmaglie/go.rice that referenced this pull request Sep 15, 2020
cmaglie added a commit to cmaglie/go.rice that referenced this pull request Sep 15, 2020
cmaglie added a commit to cmaglie/arduino-cli that referenced this pull request Sep 15, 2020
Golang projects that uses the CLI as-a-library will fail to build
because the "replace" directive is valid only for the local project.
This should fix the build until GeertJohan/go.rice#159
is merged.

Once the upstrea patch is merged this commit could be reverted and the
fork removed.
cmaglie added a commit to arduino/arduino-cli that referenced this pull request Sep 15, 2020
Golang projects that use the CLI as-a-library will fail to build
because the "replace" directive is valid only for the local project.
This should fix the build until GeertJohan/go.rice#159
is merged.

Once the upstream patch is merged this commit could be reverted and the
fork removed.
@silvanocerza
Copy link

@GeertJohan is there any chance this will be merged?

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

Successfully merging this pull request may close these issues.

None yet

3 participants