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

Fixes #191, crash on .xlsx written by non-Excel tools with non-standard xml attributes #229

Merged
merged 3 commits into from Jan 19, 2017

Conversation

Projects
None yet
2 participants
@jennybc
Member

jennybc commented Jan 17, 2017

First fruit of my C++ tutoring from @jimhester.

Prevents attempt to read value of a nonexistent attribute (can occur in xlsx written by something other than Excel).

jennybc added some commits Jan 17, 2017

Don't access value of nonexistent numFmtId attribute; fixes #191
Examples provided privately via email. "crashing" is a workbook written by <http://epplus.codeplex.com/>. "working" is same workbook, opened and re-saved from Excel.

Relevant bits of xl/styles.xml:

crashing:

<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main">
<numFmts count="0"/>
<fonts count="2">...</fonts>
<fills count="2">...</fills>
<borders count="1">...</borders>
<cellStyleXfs count="1">
<xf fontId="0"/>
</cellStyleXfs>
<cellXfs count="2">
<xf fontId="0" applyFont="1" xfId="0"/>
<xf fontId="1" applyFont="1" xfId="0"/>
</cellXfs>
<cellStyles count="1">...</cellStyles>
<dxfs count="0"/>
</styleSheet>

working:
<styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main" xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:x14ac="http://schemas.microsoft.com/office/spreadsheetml/2009/9/ac" mc:Ignorable="x14ac">
<fonts count="2">...</fonts>
<fills count="2">...</fills>
<borders count="1">...</borders>
<cellStyleXfs count="1">
<xf numFmtId="0" fontId="0" fillId="0" borderId="0"/>
</cellStyleXfs>
<cellXfs count="2">
<xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0" applyFont="1"/>
<xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/>
</cellXfs>
<cellStyles count="1">...</cellStyles>
<dxfs count="0"/>
<tableStyles count="0" defaultTableStyle="TableStyleMedium2" defaultPivotStyle="PivotStyleLight16"/>
<extLst>...</extLst>
</styleSheet>
@hadley

Can you please also a bullet to NEWS.md?

if (isDateTime(formatId, customDateFormats))
dateStyles_.insert(i);
++i;
if (cellXf->first_attribute("numFmtId") != NULL) {

This comment has been minimized.

@hadley

hadley Jan 17, 2017

Member

I think this is easier to read if you skip the bad inputs:

if (cellXf->first_attribute("numFmtId") == NULL)
  next;

This comment has been minimized.

@jennybc

jennybc Jan 17, 2017

Member

Done. Unless you want the else{...}? And bullet added to NEWS.

@jennybc

This comment has been minimized.

Member

jennybc commented Jan 17, 2017

The appveyor failure is spurious, I think. It's refusing to attempt the build vs. building and failing. I can't tell why it thinks this PR is un-mergeable.

@hadley

This comment has been minimized.

Member

hadley commented Jan 18, 2017

LGTM

@jennybc jennybc changed the title from Fixes #191 to Fixes #191, crash on .xlsx written by non-Excel tools with non-standard xml attributes Jan 19, 2017

@jennybc jennybc merged commit a1e190d into tidyverse:master Jan 19, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jennybc jennybc deleted the jennybc:iss191 branch Jan 19, 2017

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