Skip to content

overflow numeric cell value on 386 #386

New issue
New issue
Closed
Closed
overflow numeric cell value on 386#386
@sevkin

Description

@sevkin
sevkin
opened on Apr 19, 2019
Contributor

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)

Activity

mlh758

mlh758 commented on Apr 21, 2019

@mlh758
mlh758
on Apr 21, 2019
Contributor

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

mlh758 commented on Apr 21, 2019

@mlh758
mlh758
on Apr 21, 2019
Contributor

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

psyomn commented on Jul 20, 2019

@psyomn
psyomn
on 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
mentioned this on Jul 20, 2019
sevkin

sevkin commented on Jul 20, 2019

@sevkin
sevkin
on Jul 20, 2019
ContributorAuthor

@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

mlh758 commented on Jul 20, 2019

@mlh758
mlh758
on Jul 20, 2019 · edited by mlh758
Contributor

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

sevkin commented on Jul 20, 2019

@sevkin
sevkin
on Jul 20, 2019
ContributorAuthor

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

sevkin

sevkin commented on Jul 21, 2019

@sevkin
sevkin
on Jul 21, 2019
ContributorAuthor

bug localized in styles.go/formatToInt

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

now all ok @xuri

psyomn

psyomn commented on Jul 21, 2019

@psyomn
psyomn
on 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

mlh758 commented on Jul 23, 2019

@mlh758
mlh758
on Jul 23, 2019
Contributor

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

mlh758 commented on Aug 1, 2019

@mlh758
mlh758
on Aug 1, 2019
Contributor

@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
closed this as completedin 1fc4bc5on Sep 1, 2019
nullfy
added a commit that references this issue on Oct 23, 2020

Fix qax-os#386 regression test added (qax-os#440)

98c7d8e
magicaugust
added a commit that references this issue on Mar 14, 2024

Fix qax-os#386 regression test added (qax-os#440)

1fc4bc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @sevkin@psyomn@mlh758

        Issue actions
          overflow numeric cell value on 386 · Issue #386 · qax-os/excelize