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.

Sign up for GitHub

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

Jump to bottom

Avoid exceptions when count does not match the number of elements #1

Merged
merged 5 commits into from
Aug 16, 2024
Merged

Avoid exceptions when count does not match the number of elements #1

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e2e3d3
Fixes issue #735: https://github.com/tfussell/xlnt/issues/735
doomlaur May 22, 2024
f081ca4
Added a limit for memory allocation when XLSX files contain a "count"…
doomlaur Aug 15, 2024
63b2f23
Removed digit separator from constant as it does not compile under C+…
doomlaur Aug 15, 2024
cb96443
The "count" is now only parsed if necessary (that is, in certain case…
doomlaur Aug 16, 2024
053a6e1
Added a utility function for clipping the number of elements for func…
doomlaur Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions source/detail/constants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ const column_t constants::max_column()
return column_t(std::numeric_limits<column_t::index_t>::max());
}

const size_t constants::max_elements_for_reserve()
{
return 10000;
}

// constants
const path constants::package_properties()
{
Expand Down
8 changes: 8 additions & 0 deletions source/detail/constants.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ struct XLNT_API constants
/// </summary>
static const column_t max_column();

/// <summary>
/// Returns the maximum amount of elements that functions like std::vector::reserve are allowed to allocate.
/// Information like a "count" is often saved in XLSX files and can be used by std::vector::reserve to
/// allocate the memory right away and thus improve performance. However, malicious or broken files
/// might then cause XLNT to allocate extreme amounts of memory. This function sets a limit to protect against such issues.
/// </summary>
static const size_t max_elements_for_reserve();

/// <summary>
/// Returns the URI of the directory containing package properties.
/// </summary>
Expand Down
123 changes: 102 additions & 21 deletions source/detail/serialization/xlsx_consumer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ xlnt::detail::Cell parse_cell(xlnt::row_t row_arg, xml::parser *parser, std::uno
case xml::parser::end_attribute:
case xml::parser::eof:
default: {
throw xlnt::exception("unexcpected XML parsing event");
throw xlnt::exception("unexpected XML parsing event");
}
}
// Prevents unhandled exceptions from being triggered.
Expand Down Expand Up @@ -344,7 +344,7 @@ std::pair<xlnt::row_properties, int> parse_row(xml::parser *parser, xlnt::detail
case xml::parser::end_attribute:
case xml::parser::eof:
default: {
throw xlnt::exception("unexcpected XML parsing event");
throw xlnt::exception("unexpected XML parsing event");
}
}
}
Expand Down Expand Up @@ -382,7 +382,7 @@ Sheet_Data parse_sheet_data(xml::parser *parser, xlnt::detail::number_serialiser
case xml::parser::end_attribute:
case xml::parser::eof:
default: {
throw xlnt::exception("unexcpected XML parsing event");
throw xlnt::exception("unexpected XML parsing event");
}
}
}
Expand Down Expand Up @@ -2280,10 +2280,12 @@ void xlsx_consumer::read_shared_string_table()

expect_end_element(qn("spreadsheetml", "sst"));

#ifdef THROW_ON_INVALID_XML
if (has_unique_count && unique_count != target_.shared_strings().size())
{
throw invalid_file("sizes don't match");
}
#endif
}

void xlsx_consumer::read_shared_workbook_revision_headers()
Expand Down Expand Up @@ -2317,7 +2319,15 @@ void xlsx_consumer::read_stylesheet()
if (current_style_element == qn("spreadsheetml", "borders"))
{
auto &borders = stylesheet.borders;
auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
if (count.get() <= xlnt::constants::max_elements_for_reserve())
m7913d marked this conversation as resolved.
Show resolved Hide resolved
{
borders.reserve(count.get());
}
}

while (in_element(qn("spreadsheetml", "borders")))
{
Expand Down Expand Up @@ -2370,15 +2380,25 @@ void xlsx_consumer::read_stylesheet()
expect_end_element(qn("spreadsheetml", "border"));
}

if (count != borders.size())
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != borders.size())
{
throw xlnt::exception("border counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "fills"))
{
auto &fills = stylesheet.fills;
auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
if (count.get() <= xlnt::constants::max_elements_for_reserve())
{
fills.reserve(count.get());
}
}

while (in_element(qn("spreadsheetml", "fills")))
{
Expand Down Expand Up @@ -2455,15 +2475,25 @@ void xlsx_consumer::read_stylesheet()
expect_end_element(qn("spreadsheetml", "fill"));
}

if (count != fills.size())
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != fills.size())
{
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "fonts"))
{
auto &fonts = stylesheet.fonts;
auto count = parser().attribute<std::size_t>("count", 0);
optional<std::size_t> count;
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
if (count.get() <= xlnt::constants::max_elements_for_reserve())
{
fonts.reserve(count.get());
}
}

if (parser().attribute_present(qn("x14ac", "knownFonts")))
{
Expand Down Expand Up @@ -2598,15 +2628,25 @@ void xlsx_consumer::read_stylesheet()
expect_end_element(qn("spreadsheetml", "font"));
}

if (count != stylesheet.fonts.size())
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != stylesheet.fonts.size())
{
// throw xlnt::exception("counts don't match");
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "numFmts"))
{
auto &number_formats = stylesheet.number_formats;
auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
if (count.get() <= xlnt::constants::max_elements_for_reserve())
{
number_formats.reserve(count.get());
}
}

while (in_element(qn("spreadsheetml", "numFmts")))
{
Expand All @@ -2629,14 +2669,24 @@ void xlsx_consumer::read_stylesheet()
number_formats.push_back(nf);
}

if (count != number_formats.size())
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != number_formats.size())
{
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "cellStyles"))
{
auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
if (count.get() <= xlnt::constants::max_elements_for_reserve())
{
styles.reserve(count.get());
}
}

while (in_element(qn("spreadsheetml", "cellStyles")))
{
Expand Down Expand Up @@ -2665,16 +2715,33 @@ void xlsx_consumer::read_stylesheet()
expect_end_element(qn("spreadsheetml", "cellStyle"));
}

if (count != styles.size())
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != styles.size())
{
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "cellStyleXfs")
|| current_style_element == qn("spreadsheetml", "cellXfs"))
{
auto in_style_records = current_style_element.name() == "cellStyleXfs";
auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
if (count.get() <= xlnt::constants::max_elements_for_reserve())
{
if (in_style_records)
{
style_records.reserve(count.get());
}
else
{
format_records.reserve(count.get());
}
}
}

while (in_element(current_style_element))
{
Expand Down Expand Up @@ -2803,15 +2870,21 @@ void xlsx_consumer::read_stylesheet()
expect_end_element(qn("spreadsheetml", "xf"));
}

if ((in_style_records && count != style_records.size())
|| (!in_style_records && count != format_records.size()))
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && ((in_style_records && count != style_records.size())
|| (!in_style_records && count != format_records.size())))
{
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "dxfs"))
{
auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
doomlaur marked this conversation as resolved.
Show resolved Hide resolved
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
}
std::size_t processed = 0;

while (in_element(current_style_element))
Expand All @@ -2822,17 +2895,23 @@ void xlsx_consumer::read_stylesheet()
++processed;
}

if (count != processed)
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != processed)
{
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "tableStyles"))
{
skip_attribute("defaultTableStyle");
skip_attribute("defaultPivotStyle");

auto count = parser().attribute<std::size_t>("count");
optional<std::size_t> count;
doomlaur marked this conversation as resolved.
Show resolved Hide resolved
if (parser().attribute_present("count"))
{
count = parser().attribute<std::size_t>("count");
}
std::size_t processed = 0;

while (in_element(qn("spreadsheetml", "tableStyles")))
Expand All @@ -2843,10 +2922,12 @@ void xlsx_consumer::read_stylesheet()
++processed;
}

if (count != processed)
#ifdef THROW_ON_INVALID_XML
if (count.is_set() && count != processed)
{
throw xlnt::exception("counts don't match");
}
#endif
}
else if (current_style_element == qn("spreadsheetml", "extLst"))
{
Expand Down
Binary file added tests/data/Issue735_wrong_count.xlsx
View file
Open in desktop
Binary file not shown.
8 changes: 8 additions & 0 deletions tests/workbook/serialization_test_suite.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ class serialization_test_suite : public test_suite
register_test(test_Issue445_inline_str_streaming_read);
register_test(test_Issue492_stream_empty_row);
register_test(test_Issue503_external_link_load);
register_test(test_Issue735_wrong_count);
register_test(test_formatting);
register_test(test_active_sheet);
}
Expand Down Expand Up @@ -762,6 +763,13 @@ class serialization_test_suite : public test_suite
auto cell = ws.cell("A1");
xlnt_assert_equals(cell.value<std::string>(), std::string("WDG_IC_00000003.aut"));
}

void test_Issue735_wrong_count()
{
xlnt::workbook wb;
wb.load(path_helper::test_file("Issue735_wrong_count.xlsx"));
xlnt_assert_throws_nothing(wb.active_sheet());
}

void test_formatting()
{
Expand Down