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

Use uint32_t to index into xls string table; fixes #129 #293

Merged
merged 1 commit into from Mar 19, 2017
Merged

Use uint32_t to index into xls string table; fixes #129 #293

merged 1 commit into from Mar 19, 2017

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Mar 19, 2017

libxls clearly anticipates that the string table size might require an unsigned 32 bit integer, e.g.:

typedef struct SST
{
DWORD num;
DWORD numofstr;
BYTE strings;
}
SST;

typedef struct st_sst
{
DWORD count;
DWORD lastid;
DWORD continued;
DWORD lastln;
DWORD lastrt;
DWORD lastsz;
struct str_sst_string
{
BYTE* str;
}
* string;
}
st_sst;

void xls_addSST(xlsWorkBook* pWB,SST* sst,DWORD size)

void xls_appendSST(xlsWorkBook* pWB,BYTE* buf,DWORD size)

(all these types are defined here)

But then it was indexing into the table with a 16 bit integer and therefore vulnerable to getting the wrong strings. It takes a large sheet to expose this, i.e. more than 65535 unique strings. Which is the case with the xls from #129.

Two questions:

  • Should I add a test for this? It would require a very large xls, plus it doesn't really seem like something readxl should be responsible for.
  • Should I try to push this change back into libxls? It's not trivial for me, because it's on sourceforge and uses svn, whereas I've only done this with Git/GitHub. Yet it's a tiny change and seems like a no-brainer.

@jennybc jennybc requested a review from hadley March 19, 2017 06:35
@hadley
Copy link
Member

hadley commented Mar 19, 2017

I have no idea how you figured this out — impressive!

I don't think you need to worry about a test or pushing back to libxls. You could file an issue, pointing to this fix.

@jennybc
Copy link
Member Author

jennybc commented Mar 19, 2017

I have no idea how you figured this out

Let's just say the cumulative edit of 6 characters does not reflect the cumulative effort 😅.

@jimhester
Copy link
Contributor

FWIW I think you should absolutely send a patch upstream for future changes. To do so

  1. svn checkout https://svn.code.sf.net/p/libxls/code/trunk libxls-code
  2. Make your changes locally
  3. Use svn diff > my.patch to create a patch, which you can send to them.

There is also a github mirror (https://github.com/svn2github/libxls) you can use, just make your changes and git diff > my.patch although you will have to edit the patch to apply cleanly with svn.

@jennybc
Copy link
Member Author

jennybc commented Mar 30, 2017

Thanks @jimhester. This particular change has already been incorporated based just on a sourceforge exchange.

https://sourceforge.net/p/libxls/bugs/28/
https://sourceforge.net/p/libxls/code/175/

But I will do it properly in future.

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