[GH-ISSUE #525] Segfault: scatter series with no categories, when the ranges are set with chart_series_set_values() #524

New issue

Open

opened 2026-10-02 23:11:49 -06:00 by gitea-mirror · 2 comments

gitea-mirror commented

2026-10-02 23:11:49 -06:00

Owner

Originally created by @billdenney on GitHub (Jul 31, 2026).
Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/525

chart_add_series() guards against a scatter series without categories:

/* Scatter charts require categories and values. */
if (self->chart_group == LXW_CHART_SCATTER && values && !categories) {
    LXW_WARN("chart_add_series(): scatter charts must have "
             "'categories' and 'values'");
    return NULL;
}

The guard only fires when the range is passed as a string. chart.h documents an alternative:

Alternatively you can pass NULL to categories and values and use the chart_series_set_categories() and chart_series_set_values() functions

With both arguments NULL the condition values && !categories is false, so the check is skipped. If chart_series_set_categories() is then not called, workbook_close() segfaults. This affects worksheets and chartsheets alike:

/* worksheet */
lxw_chart *chart = workbook_add_chart(wb, LXW_CHART_SCATTER);
lxw_chart_series *s = chart_add_series(chart, NULL, NULL);
chart_series_set_values(s, "Sheet1", 0, 1, 4, 1);   /* no categories */
worksheet_insert_chart(ws, 0, 3, chart);
workbook_close(wb);                                  /* SIGSEGV */

/* chartsheet */
lxw_chartsheet *cs = workbook_add_chartsheet(wb, NULL);
lxw_chart *chart = workbook_add_chart(wb, LXW_CHART_SCATTER);
lxw_chart_series *s = chart_add_series(chart, NULL, NULL);
chart_series_set_values(s, "Sheet1", 0, 1, 4, 1);   /* no categories */
chartsheet_set_chart(cs, chart);
workbook_close(wb);                                  /* SIGSEGV */

All five scatter subtypes are affected.

Where

_chart_write_x_val() in src/chart.c passes the categories range on without checking ->formula:

_chart_write_x_val(lxw_chart *self, lxw_chart_series *series)
{
    uint8_t has_string_cache = series->categories->has_string_cache;
    lxw_xml_start_tag(self->file, "c:xVal", NULL);
    _chart_write_data_cache(self, series->categories, has_string_cache);
    ...

leading to _chart_write_num_ref() → _chart_write_f(self, NULL) → lxw_xml_data_element(..., data = NULL, ...) → _fprint_escaped_data() → strpbrk(NULL, "&<>").

The non-scatter path does not crash, because _chart_write_cat() returns early:

    /* Ignore <c:cat> elements for charts without category values. */
    if (!series->categories->formula)
        return;

Suggested fix

worksheet_insert_chart_opt() and chartsheet_set_chart_opt() each already walk the series list checking values, so the categories check fits in the loop that is already there — three conditions in each of the two functions:

             return LXW_ERROR_PARAMETER_VALIDATION;
         }
+
+        if (chart->chart_group == LXW_CHART_SCATTER
+            && !series->categories->formula
+            && !series->categories->sheetname) {
+            LXW_WARN("worksheet_insert_chart()/_opt(): scatter charts must "
+                     "have a 'categories' series.");
+
+            return LXW_ERROR_PARAMETER_VALIDATION;
+        }
     }

and the same in chartsheet.c with the chartsheet_set_chart()/_opt(): prefix.

I built 1.2.4 with both hunks applied and re-ran the attached reprex: the four crashing cases print the warning and workbook_close() returns 0, a scatter series with categories still writes an identical file, and the other cases are unchanged.

Checking the return value is the whole test:

lxw_chart *chart = workbook_add_chart(workbook, LXW_CHART_SCATTER);
lxw_chart_series *series = chart_add_series(chart, NULL, NULL);
chart_series_set_values(series, "Sheet1", 0, 1, 4, 1);

ASSERT_EQUAL(LXW_ERROR_PARAMETER_VALIDATION,
             worksheet_insert_chart(worksheet, 0, 3, chart));

I haven't run your test suite, so I don't know where you'd want that — the unit tests I looked at are XML comparisons, which don't fit a return-code check.

Putting the check at insert time rather than adding a NULL guard to _chart_write_x_val() is deliberate: a guard in the writer would stop the crash but emit a chart Excel cannot open, which seems worse than refusing.

Not raising this as a PR — take or leave the patch.

Reprex

lxw_scatter_segfault.c: 13 cases, one per run, so a crash does not hide the others.

gcc -O0 -g -o reprex reprex.c -lxlsxwriter -lz
for i in $(seq 1 13); do ./reprex $i; echo "case $i -> $?"; done

Cases 1, 11, 12 and 13 exit 139; the rest exit 0. The others confirm that the string form of the same mistake, missing values, missing both, NULL sheet names for the three *_set_name_range() functions, and a NULL custom-label value are all handled — this is the only path I found that crashes.

Related: #486 was a different NULL dereference in chart processing (invalid chart type 0), fixed on main.

Found while adding chart support to the writexl R package, which builds series through the programmatic idiom throughout.

Originally created by @billdenney on GitHub (Jul 31, 2026). Original GitHub issue: https://github.com/jmcnamara/libxlsxwriter/issues/525 `chart_add_series()` guards against a scatter series without categories: ```c /* Scatter charts require categories and values. */ if (self->chart_group == LXW_CHART_SCATTER && values && !categories) { LXW_WARN("chart_add_series(): scatter charts must have " "'categories' and 'values'"); return NULL; } ``` The guard only fires when the range is passed as a string. `chart.h` documents an alternative: > Alternatively you can pass `NULL` to `categories` and `values` and use the `chart_series_set_categories()` and `chart_series_set_values()` functions With both arguments `NULL` the condition `values && !categories` is false, so the check is skipped. If `chart_series_set_categories()` is then not called, `workbook_close()` segfaults. This affects **worksheets and chartsheets alike**: ```c /* worksheet */ lxw_chart *chart = workbook_add_chart(wb, LXW_CHART_SCATTER); lxw_chart_series *s = chart_add_series(chart, NULL, NULL); chart_series_set_values(s, "Sheet1", 0, 1, 4, 1); /* no categories */ worksheet_insert_chart(ws, 0, 3, chart); workbook_close(wb); /* SIGSEGV */ ``` ```c /* chartsheet */ lxw_chartsheet *cs = workbook_add_chartsheet(wb, NULL); lxw_chart *chart = workbook_add_chart(wb, LXW_CHART_SCATTER); lxw_chart_series *s = chart_add_series(chart, NULL, NULL); chart_series_set_values(s, "Sheet1", 0, 1, 4, 1); /* no categories */ chartsheet_set_chart(cs, chart); workbook_close(wb); /* SIGSEGV */ ``` All five scatter subtypes are affected. ### Where `_chart_write_x_val()` in `src/chart.c` passes the categories range on without checking `->formula`: ```c _chart_write_x_val(lxw_chart *self, lxw_chart_series *series) { uint8_t has_string_cache = series->categories->has_string_cache; lxw_xml_start_tag(self->file, "c:xVal", NULL); _chart_write_data_cache(self, series->categories, has_string_cache); ... ``` leading to `_chart_write_num_ref()` → `_chart_write_f(self, NULL)` → `lxw_xml_data_element(..., data = NULL, ...)` → `_fprint_escaped_data()` → `strpbrk(NULL, "&<>")`. The non-scatter path does not crash, because `_chart_write_cat()` returns early: ```c /* Ignore <c:cat> elements for charts without category values. */ if (!series->categories->formula) return; ``` ### Suggested fix `worksheet_insert_chart_opt()` and `chartsheet_set_chart_opt()` each already walk the series list checking `values`, so the categories check fits in the loop that is already there — three conditions in each of the two functions: ```diff return LXW_ERROR_PARAMETER_VALIDATION; } + + if (chart->chart_group == LXW_CHART_SCATTER + && !series->categories->formula + && !series->categories->sheetname) { + LXW_WARN("worksheet_insert_chart()/_opt(): scatter charts must " + "have a 'categories' series."); + + return LXW_ERROR_PARAMETER_VALIDATION; + } } ``` and the same in `chartsheet.c` with the `chartsheet_set_chart()/_opt():` prefix. I built 1.2.4 with both hunks applied and re-ran the attached reprex: the four crashing cases print the warning and `workbook_close()` returns 0, a scatter series *with* categories still writes an identical file, and the other cases are unchanged. Checking the return value is the whole test: ```c lxw_chart *chart = workbook_add_chart(workbook, LXW_CHART_SCATTER); lxw_chart_series *series = chart_add_series(chart, NULL, NULL); chart_series_set_values(series, "Sheet1", 0, 1, 4, 1); ASSERT_EQUAL(LXW_ERROR_PARAMETER_VALIDATION, worksheet_insert_chart(worksheet, 0, 3, chart)); ``` I haven't run your test suite, so I don't know where you'd want that — the unit tests I looked at are XML comparisons, which don't fit a return-code check. Putting the check at insert time rather than adding a NULL guard to `_chart_write_x_val()` is deliberate: a guard in the writer would stop the crash but emit a chart Excel cannot open, which seems worse than refusing. Not raising this as a PR — take or leave the patch. ### Reprex [lxw_scatter_segfault.c](https://github.com/user-attachments/files/30587868/lxw_scatter_segfault.c): 13 cases, one per run, so a crash does not hide the others. ``` gcc -O0 -g -o reprex reprex.c -lxlsxwriter -lz for i in $(seq 1 13); do ./reprex $i; echo "case $i -> $?"; done ``` Cases 1, 11, 12 and 13 exit 139; the rest exit 0. The others confirm that the string form of the same mistake, missing values, missing both, NULL sheet names for the three `*_set_name_range()` functions, and a NULL custom-label value are all handled — this is the only path I found that crashes. Related: #486 was a different NULL dereference in chart processing (invalid chart type `0`), fixed on main. Found while adding chart support to the writexl R package, which builds series through the programmatic idiom throughout.

gitea-mirror commented

2026-10-02 23:11:49 -06:00

Author

Owner

@billdenney commented on GitHub (Jul 31, 2026):

This is not causing an issue in production; it was noted during the process without major incident.

@billdenney commented on GitHub (Jul 31, 2026): This is not causing an issue in production; it was noted during the process without major incident.

gitea-mirror commented

2026-10-02 23:11:50 -06:00

Author

Owner

@jmcnamara commented on GitHub (Jul 31, 2026):

Thanks @billdenney. That looks like a bug. I'll look into it.

@jmcnamara commented on GitHub (Jul 31, 2026): Thanks @billdenney. That looks like a bug. I'll look into it.