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

overflow numeric cell value on 386 #386

Closed
sevkin opened this issue Apr 19, 2019 · 10 comments
Closed

overflow numeric cell value on 386 #386

sevkin opened this issue Apr 19, 2019 · 10 comments

Comments

@sevkin
Copy link
Contributor

sevkin commented Apr 19, 2019

Description
I am trying to read xlsx files containing ean13 codes for example 8595602512225

Sample file (attached to issue or download from gdrive)
barcode_test.xlsx
https://drive.google.com/open?id=16FswVFaJ5I1cVPVeViOTZWLOGg1b1eJz

This is important because when I simply open and save this file in libreoffice, the error is not reproduced.

Steps to reproduce the issue:

  1. Sample code
xlsx, err := excelize.OpenFile("./barcode_test.xlsx")
if err == nil {
	cell := xlsx.GetCellValue("Sheet1", "A1")
	fmt.Println(cell)
}
  1. Build instructions
export GOARCH="amd64"; go build -o et.x64
export GOARCH="386"; go build -o et.x32
  1. Execute binaries

Describe the results you received:

» ./et.x32 
-2147483648
» ./et.x64 
8595602512225

Describe the results you expected:

» ./et.x32 
8595602512225
» ./et.x64 
8595602512225

(and this output after open then save in libreoffice)

Output of go version:

go version go1.12.1 linux/amd64

Excelize version or commit ID:

require github.com/360EntSecGroup-Skylar/excelize v1.4.1

Environment details (OS, Microsoft Excel™ version, physical, etc.):

» uname -a
Linux XXX 4.15.0-47-generic #50-Ubuntu SMP Wed Mar 13 10:44:52 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Microsoft Excel™ is unknown - i just received this file
(same result on win10)

@mlh758
Copy link
Contributor

mlh758 commented Apr 21, 2019

Interesting, GetCellValue returns a string so I'm not sure why it would run through an integer first and have a chance to overflow like that.

@mlh758
Copy link
Contributor

mlh758 commented Apr 21, 2019

I wonder if this is the culprit. We're not checking any errors there and it converts to an int. Will need to test to find out.

@psyomn
Copy link

psyomn commented Jul 20, 2019

I'm guessing this has been fixed?

With latest:

$ go run test.go 
8595602512225

I am taking a look if there is a regression test written in this respect. Would that be appreciated?

@sevkin sevkin mentioned this issue Jul 20, 2019
9 tasks
@sevkin
Copy link
Contributor Author

sevkin commented Jul 20, 2019

@psyomn not fixed

$ go test ./...
ok      github.com/360EntSecGroup-Skylar/excelize/v2    1.884s
$ export GOARCH=386; go test ./...
--- FAIL: TestOverflowNumericCell (0.01s)
    cell_test.go:108: 
                Error Trace:    cell_test.go:108
                Error:          Not equal: 
                                expected: "8595602512225"
                                actual  : "-2147483648"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -8595602512225
                                +-2147483648
                Test:           TestOverflowNumericCell
                Messages:       A1 should be 8595602512225
FAIL
FAIL    github.com/360EntSecGroup-Skylar/excelize/v2    2.060s

@mlh758
Copy link
Contributor

mlh758 commented Jul 20, 2019

Would you be able to PR this test into the repo? It would be helpful for making a fix and preventing a regression later.

Edit: Never mind, I see there is already an open PR.

@sevkin
Copy link
Contributor Author

sevkin commented Jul 20, 2019

hmm travis - tests passed, my laptop - failed
i dont understant it

@sevkin
Copy link
Contributor Author

sevkin commented Jul 21, 2019

bug localized in styles.go/formatToInt

- return fmt.Sprintf("%d", int(f))
+ return fmt.Sprintf("%d", int64(f))

now all ok @xuri

@psyomn
Copy link

psyomn commented Jul 21, 2019

If I'm reading that correctly, that might be a good workaround for now, but I wonder what happens if we get even bigger numbers than (u)int64, or what the correct thing to do would be. Seems that fairly large numbers are supported in Excel:

https://support.office.com/en-us/article/Excel-specifications-and-limits-1672b34d-7043-467e-8e27-269d656771c3

@mlh758
Copy link
Contributor

mlh758 commented Jul 23, 2019

fairly large numbers

That’s an understatement if I’m reading the docs right.

I wonder if any of the crypto libraries have support for efficient big numbers.

@mlh758
Copy link
Contributor

mlh758 commented Aug 1, 2019

@xuri Maybe for performance reasons and our own sanity we specify that the maximum numeric value we support is an int64? That should cover a substantial majority of use cases, and we can investigate broadening that support more carefully if it comes up. I'd be in favor of closing this issue now that we are at least properly specifying the integer type.

@xuri xuri closed this as completed in 1fc4bc5 Sep 1, 2019
nullfy pushed a commit to nullfy/excelize that referenced this issue Oct 23, 2020
* qax-os#386 regression test added

* closes qax-os#386 string to bigint on GOARCH=386
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

No branches or pull requests

3 participants