Pull upstream changes from libxls #442

jennybc · 2018-04-12T21:21:40Z

Fixes #441 Incorporate upstream changes to libxls to address security vulnerabilities

Syncs with libxls as of libxls/libxls@c7e5e49

jennybc · 2018-04-12T21:27:15Z

Here's the previous PR I used to reconcile libxls with what we have here:

#329

I'm going to transfer knowledge from there to here

jennybc · 2018-04-12T21:37:09Z

src/libxls/ole.h

@@ -37,7 +37,7 @@

 #include "libxls/xlstypes.h"

-#if defined(_AIX) || defined(__sun)
+#ifdef AIX


This change and carbon copies thereof (one more in ole.h then one each in xlsstruct.h, xls.c, and xlstool.c) were made by @hadley in 6f8304c "Attempted fixes for CRAN issues".

jennybc · 2018-04-12T21:37:36Z

src/libxls/ole.h

@@ -129,15 +140,15 @@ typedef struct OLE2Stream
 }
 OLE2Stream;

-#if defined(_AIX) || defined(__sun)
+#ifdef AIX
 #pragma pack(1)


See explanation at first instance of this in ole.h.

jennybc · 2018-04-12T21:40:22Z

src/libxls/xlsstruct.h

@@ -80,7 +80,7 @@

 #define BLANK_CELL  XLS_RECORD_BLANK  // compat

-#if defined(_AIX) || defined(__sun)
+#ifdef AIX
 #pragma pack(1)


See explanation at first instance of this in ole.h.

jennybc · 2018-04-12T21:42:07Z

src/libxls/xlsstruct.h

@@ -124,7 +123,7 @@ typedef struct BOUNDSHEET
    DWORD	filepos;
    BYTE	type;
    BYTE	visible;
-    BYTE	name[1];
+    char	name[];
 }


@hadley previously said: I have a vague memory that these were necessary to support the version of C/C++ we use

This change and several other instances of [] -> [1] and [0] -> [1] were made by @hadley in 6f8304c "Attempted fixes for CRAN issues". Multiple instances here in xlsstruct.h as well as several more in xls.c.

If I ever want to read more about this, this SO thread looks like the place to start. It seems to be about "flexible array members" and, yes, what's kosher varies by compiler.

What's the need of array with zero elements?

jennybc · 2018-04-12T21:42:37Z

src/libxls/xlsstruct.h

@@ -195,7 +194,7 @@ typedef struct MULRK
 	struct {
 		WORD	xf;
 		DWORD_UA value;
-	}		rk[1];
+	}		rk[];


See explanation at first instance of this in xlsstruct.h.

jennybc · 2018-04-12T21:42:48Z

src/libxls/xlsstruct.h

@@ -204,7 +203,7 @@ typedef struct MULBLANK
 {
    WORD	row;
    WORD	col;
-    WORD	xf[1];
+    WORD	xf[];


See explanation at first instance of this in xlsstruct.h.

jennybc · 2018-04-12T21:42:58Z

src/libxls/xlsstruct.h

@@ -222,7 +221,7 @@ typedef struct LABEL
    WORD	row;
    WORD	col;
    WORD	xf;
-    BYTE	value[1]; // var
+    BYTE	value[]; // var
 }


See explanation at first instance of this in xlsstruct.h.

jennybc · 2018-04-12T21:44:31Z

src/libxls/xlsstruct.h

 }
 FONT;

 typedef struct FORMAT
 {
    WORD	index;
-    BYTE	value[1];
+    char	value[];
 }


See explanation at first instance of this in xlsstruct.h.

jennybc · 2018-04-12T21:45:48Z

src/libxls/xlstool.h

@@ -31,15 +31,13 @@
 */

 #include "libxls/xlsstruct.h"
-/* Mask illegal functions for CMD check */
-#include "cran.h"


From previous reconciliation:

cran.h is also included in ole.c, prefaced with this comment:

/* Mask illegal functions for CMD check */

From the travis logs on the first commit in this PR, I gather that the inclusion of cran.h is aimed at preventing these errors:

* checking compiled code ... WARNING File ‘readxl/libs/readxl.so’: Found ‘exit’, possibly from ‘exit’ (C) Object: ‘ole.o’ Found ‘putchar’, possibly from ‘putchar’ (C) Object: ‘xls.o’ Found ‘puts’, possibly from ‘printf’ (C), ‘puts’ (C) Objects: ‘ole.o’, ‘xls.o’, ‘xlstool.o’ Found ‘stderr’, possibly from ‘stderr’ (C) Object: ‘ole.o’ Compiled code should not call entry points which might terminate R nor write to stdout/stderr instead of to the console, nor the system RNG. See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual.

From the content of cran.h, it is obvious to me why it suppresses warnings re: exit and printf. It is not obvious to me how it has any effect on the warnings re: putchar and stderr.

jennybc · 2018-04-12T21:47:25Z

src/libxls/xlstool.h

@@ -49,6 +47,6 @@ extern void xls_showCell(struct st_cell_data* cell);
 extern void xls_showFont(struct st_font_data* font);
 extern void xls_showXF(XF8* xf);
 extern void xls_showFormat(struct st_format_data* format);
-extern BYTE* xls_getfcell(xlsWorkBook* pWB,struct st_cell_data* cell,BYTE *label);
+extern char* xls_getfcell(xlsWorkBook* pWB, struct st_cell_data* cell, WORD *label);


Updated signature for xls_getfcell(): label should be pointer to BYTE instead of DWORD (recent-ish hack from Jenny, semi-unfortunately already accepted upstream) or WORD (as we found libxls, also incorrect, and what Jenny was trying to fix).

This is one of the changes from @jimhester that I believe should go upstream.

Discussion here: #312 (comment)

readxl commit making this change: 357e473

Patch proposed to libxls: https://sourceforge.net/p/libxls/patches/13/

Part of libxls/libxls#10

jennybc · 2018-04-12T21:48:33Z

src/ole.c

 #include "libxls/ole.h"
 #include "libxls/xlstool.h"
 #include "libxls/endian.h"
-/* Mask illegal functions for CMD check */
-#include "cran.h"


See explanation upon first instance of this in xlstool.h.

jennybc · 2018-04-12T21:50:43Z

src/ole.c


-			assert(olest->fatpos >= 0);


Came up in last reconciliation but I think we just accept/evaluate the larger change here.
Previous notes: 33dd9a0#r111018675

jennybc · 2018-04-12T21:56:30Z

src/xls.c


 #ifndef min
 #define min(a,b) ((a) < (b) ? (a) : (b))
 #endif

-// #define DEBUG_DRAWINGS
+//#define DEBUG_DRAWINGS


We had commented this out a long time ago.

jennybc · 2018-04-12T21:57:32Z

src/xls.c

-extern void xls_parseWorkSheet(xlsWorkSheet* pWS);
-extern void xls_dumpSummary(char *buf,int isSummary,xlsSummaryInfo	*pSI);
-
-#if defined(_AIX) || defined(__sun)


See explanation of just the above line at first instance of this in ole.h.

jennybc · 2018-04-12T21:58:04Z

src/xls.c

@@ -91,7 +90,7 @@ typedef struct {
 	uint32_t		os;
 	uint32_t		format[4];
 	uint32_t		count;
-	sectionList		secList[1];
+	sectionList		secList[0];


See explanation at first instance of this in xlsstruct.h.

jennybc · 2018-04-12T22:05:22Z

src/xls.c

-		sscanf((char *)cell->str, "%lf", &cell->d);
+        if (bof->size < sizeof(LABEL))
+            return NULL;
+		cell->str=xls_getfcell(pWS->workbook,cell,(WORD_UA *)&((LABEL*)buf)->value);


See above. We want the BYTE version. I believe this is a case where readxl has a change to send upstream.

Part of libxls/libxls#10

jennybc · 2018-04-12T22:13:57Z

src/xls.c

@@ -1453,7 +1708,7 @@ xlsCell	*xls_cell(xlsWorkSheet* pWS, WORD cellRow, WORD cellCol)

    if(cellRow > pWS->rows.lastrow) return NULL;
    row = &pWS->rows.row[cellRow];
-    if(cellCol > row->lcell) return NULL;
+    if(cellCol >= row->lcell) return NULL;


This was me and we want the > version. However, the situation is murky enough that it's not clear this change should go upstream.

92b7b16

I should definitely test w/o this change, given all the other changes in this file. Perhaps the root problem has been addressed in a different way.

jennybc · 2018-04-12T22:15:21Z

src/xlstool.c

@@ -57,8 +57,6 @@

 extern int xls_debug;

-// static void xls_showBOUNDSHEET(void* bsheet);
-


We had deleted. I changed to commenting out. So take this deletion.

33dd9a0#r111037267

jennybc · 2018-04-12T22:16:53Z

src/xlstool.c

-#endif
-
-#endif
-



We had edits in the above code FWIW:

33dd9a0#r111038179

jennybc · 2018-04-12T22:17:21Z

src/xlstool.c

 {
 #ifdef HAVE_ICONV
 	// Do iconv conversion
-#if defined(_AIX) || defined(__sun)
+#ifdef AIX
    const char *from_enc = "UTF-16le";


See explanation at first instance of this in ole.h.

jennybc · 2018-04-12T22:17:59Z

src/xlstool.c

                st = iconv(ic, (char **)&src_ptr, &inlenleft, (char **)&out_ptr,(size_t *) &outlenleft);
-#endif
                if(st == (size_t)(-1))


These lines added in 81443a1 "Fix remaining windows/R CMD check problems", along with a very similar change in endian.c.

jennybc · 2018-04-12T22:18:38Z

src/xlstool.c

@@ -628,78 +559,89 @@ void xls_showXF(XF8* xf)
    printf("GroundColor: 0x%x\n",xf->groundcolor);
 }

-BYTE *xls_getfcell(xlsWorkBook* pWB,struct st_cell_data* cell,BYTE *label)
+char *xls_getfcell(xlsWorkBook* pWB, struct st_cell_data* cell, WORD *label)
 {


See main discussion of this change to xls_getfcell() in xlstoolh.h.

Part of libxls/libxls#10

jennybc · 2018-04-12T22:22:09Z

src/xlstool.c

        break;
    case XLS_RECORD_BLANK:
    case XLS_RECORD_MULBLANK:
-        asprintf(&ret, "%s", "");
+        ret = strdup("");
        break;


We already/also differed from upstream here. Note that asprintf() has been deleted above.

33dd9a0#r111039981

jennybc · 2018-04-12T22:22:33Z

src/xlstool.c

        break;
    case XLS_RECORD_LABEL:
 		len = xlsShortVal(*label);
-        label += 2;
+        label++;
 		if(pWB->is5ver) {


See main discussion of this change to xls_getfcell() in xlstoolh.h.

Part of libxls/libxls#10

jennybc · 2018-04-12T23:58:12Z

src/ole.c

@@ -38,13 +38,9 @@
 #include <stdio.h>
 #include <stdlib.h>

-#include <assert.h>
-


Related to recent changes in libxls

Eliminate exits and asserts. Hooray!
Libraries should not exit. Rework internal API to bubble errors up instead of exiting.
libxls/libxls@c36e88d

jennybc · 2018-04-12T23:58:45Z

src/xls.c

@@ -41,40 +41,39 @@
 #include <sys/types.h>
 #include <string.h>
 #include <wchar.h>
-#include <assert.h>


Related to recent changes in libxls

Eliminate exits and asserts. Hooray!
Libraries should not exit. Rework internal API to bubble errors up instead of exiting.
libxls/libxls@c36e88d

92b7b16

jennybc · 2018-04-14T01:26:05Z

src/endian.c

-      b->min_ver = xlsIntVal(b->min_ver);
-    }
+    b->flags = xlsIntVal(b->flags);
+    b->min_ver = xlsIntVal(b->min_ver);
 }


This is being addressed now in libxls, in a different way. We'll let go of our fix.

Pull state of overlapping files from libxls/libxls@3a2acf1

748095f

jennybc commented Apr 12, 2018

View reviewed changes

src/xlstool.c

#endif

#endif

Copy link

Member Author

jennybc Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had edits in the above code FWIW:

33dd9a0#r111038179

jennybc commented Apr 12, 2018

View reviewed changes

jennybc added 2 commits April 12, 2018 18:39

Correct path to xls.h

b964d1c

Reinstate cran.h

8b05f52

Remove fprintf macro (for now)

24f70d4

jennybc force-pushed the libxls-pull-upstream branch from dd42105 to 24f70d4 Compare April 13, 2018 01:42

Patch xls.c to read all cols in xls written by some third-party s/w

d74e4f1

92b7b16

jennybc changed the title ~~DO NOT MERGE~~ Pull upstream changes from libxls Apr 13, 2018

jennybc added 4 commits April 13, 2018 08:21

Apply libxls/libxls@967fbd7

9bdeecd

Apply libxls/libxls@a9f823f

ce8e364

Apply libxls/libxls@7ea6ee3

94782cf

Apply libxls/libxls@c7e5e49

0945730

jennybc mentioned this pull request Apr 13, 2018

Incorporate upstream changes to libxls to address security vulnerabilities #441

Closed

jennybc added 2 commits April 13, 2018 17:41

Reinstate the fprintf macro ... several of them!

45710a0

Exploit the fprintf macros

3bcab5b

jennybc force-pushed the libxls-pull-upstream branch from e6263ab to 3bcab5b Compare April 14, 2018 00:56

jennybc commented Apr 14, 2018

View reviewed changes

Solaris woo

2e639ed

jennybc merged commit 49b4f83 into master Apr 14, 2018

jennybc deleted the libxls-pull-upstream branch April 14, 2018 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull upstream changes from libxls #442

Pull upstream changes from libxls #442

jennybc commented Apr 12, 2018 •

edited

Loading

jennybc commented Apr 12, 2018 •

edited

Loading

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 13, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018 •

edited

Loading

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 13, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 13, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 13, 2018

jennybc Apr 12, 2018

jennybc Apr 12, 2018

jennybc Apr 14, 2018

		@@ -57,8 +57,6 @@

		extern int xls_debug;

		// static void xls_showBOUNDSHEET(void* bsheet);

Pull upstream changes from libxls #442

Pull upstream changes from libxls #442

Conversation

jennybc commented Apr 12, 2018 • edited Loading

jennybc commented Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc commented Apr 12, 2018 •

edited

Loading

jennybc commented Apr 12, 2018 •

edited

Loading

jennybc Apr 12, 2018 •

edited

Loading