-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 panic in getSheetMap #1437
Fix panic in getSheetMap #1437
Conversation
Add missing error checks in `getSheetMap`
Codecov Report
@@ Coverage Diff @@
## master #1437 +/- ##
==========================================
- Coverage 98.57% 98.56% -0.02%
==========================================
Files 31 31
Lines 24036 24044 +8
==========================================
+ Hits 23694 23699 +5
- Misses 226 228 +2
- Partials 116 117 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your pull request. I think the attachment CorruptedRel.xlsx
and the test function TestCorruptedRelationship
can be removed, the unit tests can be simplified by this patch based on the currently code:
diff --git a/excelize_test.go b/excelize_test.go
index 47d83a8..63d96f4 100644
--- a/excelize_test.go
+++ b/excelize_test.go
@@ -250,7 +250,11 @@ func TestOpenReader(t *testing.T) {
assert.NoError(t, zw.Close())
return buf
}
- for _, defaultXMLPath := range []string{defaultXMLPathCalcChain, defaultXMLPathStyles} {
+ for _, defaultXMLPath := range []string{
+ defaultXMLPathCalcChain,
+ defaultXMLPathStyles,
+ defaultXMLPathWorkbookRels,
+ } {
_, err = OpenReader(preset(defaultXMLPath))
assert.EqualError(t, err, "XML syntax error on line 1: invalid UTF-8")
}
diff --git a/sheet_test.go b/sheet_test.go
index c44b287..f809fe8 100644
--- a/sheet_test.go
+++ b/sheet_test.go
@@ -114,11 +114,6 @@ func TestSetPanes(t *testing.T) {
))
}
-func TestCorruptedRelationship(t *testing.T) {
- _, err := OpenFile(filepath.Join("test", "CorruptedRel.xlsx"))
- assert.Error(t, err)
-}
-
func TestSearchSheet(t *testing.T) {
f, err := OpenFile(filepath.Join("test", "SharedStrings.xlsx"))
if !assert.NoError(t, err) {
@@ -383,6 +378,12 @@ func TestGetSheetMap(t *testing.T) {
}
assert.Equal(t, len(sheetMap), 2)
assert.NoError(t, f.Close())
+
+ f = NewFile()
+ f.WorkBook = nil
+ f.Pkg.Store(defaultXMLPathWorkbook, MacintoshCyrillicCharset)
+ _, err = f.getSheetMap()
+ assert.EqualError(t, err, "XML syntax error on line 1: invalid UTF-8")
}
func TestSetActiveSheet(t *testing.T) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you very much.
Unit tests updated
Unit tests updated
Add missing error checks in
getSheetMap
PR Details
Missing error checks in
getSheetMap
Description
Add error checks for
getSheetMap
Related Issue
Motivation and Context
There is no error checks, parser can crash on malformed input
How Has This Been Tested
Tested against a sample corrupted file
Types of changes
Checklist