workbook_close stack-buffer-overflow

[wxie7]

hello, maybe there exist a bug in workbook_close. Below is an example

#include <xlsxwriter/workbook.h>
#include <xlsxwriter/worksheet.h>

int main() {

    /* Create a new workbook and add a worksheet. */
    lxw_workbook  *workbook  = workbook_new("demo.xlsx");
    lxw_worksheet *worksheet = workbook_add_worksheet(workbook, NULL);

    // lxw_format *format = workbook_add_format(workbook);
    // format_set_font_color(format, LXW_COLOR_RED);

    lxw_conditional_format cf = {
        .type = 1,
        .criteria = 12,
        .value = 0,
        .format = NULL
    };
    lxw_error err = worksheet_conditional_format_range(worksheet, RANGE("B1:B9"), &cf);
    if (err != LXW_NO_ERROR)
        return 1;

    workbook_close(workbook);

    return 0;
}

The following is asan information

=================================================================
==5078==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffdb9547900 at pc 0x561770102774 bp 0x7ffdb9547850 sp 0x7ffdb9547848
READ of size 8 at 0x7ffdb9547900 thread T0
    #0 0x561770102773 in _worksheet_write_cf_rule_cell /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:6746:5
    #1 0x561770101694 in _worksheet_write_cf_rule /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:6776:9
    #2 0x561770101353 in _worksheet_write_conditional_formatting /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:6845:9
    #3 0x5617700b69a2 in _worksheet_write_conditional_formats /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:6865:9
    #4 0x5617700b2b10 in lxw_worksheet_assemble_xml_file /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:7694:5
    #5 0x561770189491 in _write_worksheet_files /home/ubuntu/workspace/newest-libxlsxwriter/src/packager.c:311:9
    #6 0x561770186673 in lxw_create_package /home/ubuntu/workspace/newest-libxlsxwriter/src/packager.c:1894:13
    #7 0x561770062aff in workbook_close /home/ubuntu/workspace/newest-libxlsxwriter/src/workbook.c:2214:13
    #8 0x5617700518db in main /home/ubuntu/workspace/newest-libxlsxwriter/build/../bugs/bug4.cpp:24:5
    #9 0x7f8945796d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #10 0x7f8945796e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #11 0x56176ff91454 in _start (/home/ubuntu/workspace/newest-libxlsxwriter/build/bug4+0x58454) (BuildId: 45a7f2111caf46217f46fd5cd1ef23797eedaa35)

Address 0x7ffdb9547900 is located in stack of thread T0 at offset 160 in frame
    #0 0x561770101e1f in _worksheet_write_cf_rule_cell /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:6719

  This frame has 2 object(s):
    [32, 48) 'attributes' (line 6720)
    [64, 136) 'operators' (line 6722) <== Memory access at offset 160 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow /home/ubuntu/workspace/newest-libxlsxwriter/src/worksheet.c:6746:5 in _worksheet_write_cf_rule_cell
Shadow bytes around the buggy address:
  0x1000372a0ed0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0ee0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0ef0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0f00: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x1000372a0f10: 00 00 f2 f2 00 00 00 00 00 00 00 00 00 f3 f3 f3
=>0x1000372a0f20:[f3]f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x1000372a0f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==5078==ABORTING

[wxie7]

It is also worth noting that when criteria are 16, 27, 29, 31, 33, SEGV will result. There may be other criteria to consider

[jmcnamara]

It is also worth noting that when criteria are 16, 27, 29, 31, 33, SEGV will result. There may be other criteria to consider

Thanks for the report. That is a bug. There should be validation in the function to check that conditional formats that reference strings actually have a non-NULL string.

The other issues are probably similar. I'll take a look at those too.

[jmcnamara]

There should be validation in the function to check that conditional formats that reference strings actually have a non-NULL string.

It turns out that I did have validation in the code for this but the example abuses the API to use a TEXT criteria for a CELL conditional format.

The documentation for each conditional format type lists the allowable criteria (example). Nevertheless, the library should validate that as well. I'll add a new check.

[jmcnamara]

I've pushed a fix to main that checks that the conditional formatting criteria matches the conditional format type. Your example will now return a lxw_error and warning.