Newsboat: Formatting code causes SIGSEGV related to parenthesis

[rtyler]

Newsboat version 2.16.1:

Steps to reproduce the issue:

1. Add the NetBSD blog RSS feed http://blog.netbsd.org/tnf/feed/entries/atom
2. Locate and try to view this blog post:
   . https://blog.netbsd.org/tnf/entry/gsoc_2019_report_adding_netbsd1
3. SIGSEGV

I spent some try trying to find the location of what appears to be a double free, but the code that's handling formatting seems to cross the Rust/C++ boundary and gets too complex for me to follow :frowning:

I reproduced this with the latest HEAD off master too fwiw.

[Minoru]

Reproduced with c63da29f70de21f045a1e40d45bcbf332ef71a4c too. Thanks for the report!

I don't have time to dig into this right now, and later on the article might fall off the feed, so here's a copy: atom.gz

Clang's AddressSanitizer points at HtmlRenderer, says it's a heap buffer overflow. Apparently Rust is not involved here. @rtyler, have you examined this with some other tools that said it's a double free?

AddressSanitizer's report:

=================================================================
==28279==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000019f0c at pc 0x0000005aef9a bp 0x7ffd67fb0430 sp 0x7ffd67fb0428
WRITE of size 4 at 0x602000019f0c thread T0
    #0 0x5aef99 in void __gnu_cxx::new_allocator<unsigned int>::construct<unsigned int, unsigned int const&>(unsigned int*, unsigned int const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:147:4
    #1 0x5aef99 in void std::allocator_traits<std::allocator<unsigned int> >::construct<unsigned int, unsigned int const&>(std::allocator<unsigned int>&, unsigned int*, unsigned int const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/alloc_traits.h:484
    #2 0x5aef99 in std::vector<unsigned int, std::allocator<unsigned int> >::push_back(unsigned int const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:1189
    #3 0x5aef99 in newsboat::HtmlRenderer::render(std::istream&, std::vector<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/minoru/src/newsboat/src/htmlrenderer.cpp:311
    #4 0x5a0aae in newsboat::HtmlRenderer::render(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/minoru/src/newsboat/src/htmlrenderer.cpp:65:2
    #5 0x921127 in newsboat::render_html(newsboat::ConfigContainer&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) /home/minoru/src/newsboat/src/itemrenderer.cpp:94:7
    #6 0x923f5a in newsboat::item_renderer::to_stfl_list(newsboat::ConfigContainer&, std::shared_ptr<newsboat::RssItem>, unsigned int, unsigned int, newsboat::RegexManager*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&) /home/minoru/src/newsboat/src/itemrenderer.cpp:157:2
    #7 0x72a980 in newsboat::ItemViewFormAction::prepare() /home/minoru/src/newsboat/src/itemviewformaction.cpp:121:5
    #8 0x5d1078 in newsboat::View::run() /home/minoru/src/newsboat/src/view.cpp:204:7
    #9 0x6348ff in newsboat::Controller::run(newsboat::CliArgsParser const&) /home/minoru/src/newsboat/src/controller.cpp:483:15
    #10 0x518a06 in main /home/minoru/src/newsboat/newsboat.cpp:185:11
    #11 0x7f1f12bc2bba in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26bba)
    #12 0x4308e9 in _start (/home/minoru/src/newsboat/newsboat+0x4308e9)

0x602000019f0c is located 4 bytes to the left of 4-byte region [0x602000019f10,0x602000019f14)
allocated by thread T0 here:
    #0 0x50b7b2 in operator new(unsigned long) (/home/minoru/src/newsboat/newsboat+0x50b7b2)
    #1 0x5a6733 in __gnu_cxx::new_allocator<unsigned int>::allocate(unsigned long, void const*) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:114:27
    #2 0x5a6733 in std::allocator_traits<std::allocator<unsigned int> >::allocate(std::allocator<unsigned int>&, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/alloc_traits.h:444
    #3 0x5a6733 in std::_Vector_base<unsigned int, std::allocator<unsigned int> >::_M_allocate(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:343
    #4 0x5a6733 in void std::vector<unsigned int, std::allocator<unsigned int> >::_M_realloc_insert<unsigned int const&>(__gnu_cxx::__normal_iterator<unsigned int*, std::vector<unsigned int, std::allocator<unsigned int> > >, unsigned int const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/vector.tcc:440
    #5 0x5a6733 in std::vector<unsigned int, std::allocator<unsigned int> >::push_back(unsigned int const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:1195
    #6 0x5a6733 in newsboat::HtmlRenderer::render(std::istream&, std::vector<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/minoru/src/newsboat/src/htmlrenderer.cpp:311
    #7 0x5a0aae in newsboat::HtmlRenderer::render(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/minoru/src/newsboat/src/htmlrenderer.cpp:65:2
    #8 0x921127 in newsboat::render_html(newsboat::ConfigContainer&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<newsboat::LineType, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) /home/minoru/src/newsboat/src/itemrenderer.cpp:94:7
    #9 0x923f5a in newsboat::item_renderer::to_stfl_list(newsboat::ConfigContainer&, std::shared_ptr<newsboat::RssItem>, unsigned int, unsigned int, newsboat::RegexManager*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, newsboat::LinkType> > >&) /home/minoru/src/newsboat/src/itemrenderer.cpp:157:2
    #10 0x72a980 in newsboat::ItemViewFormAction::prepare() /home/minoru/src/newsboat/src/itemviewformaction.cpp:121:5
    #11 0x5d1078 in newsboat::View::run() /home/minoru/src/newsboat/src/view.cpp:204:7
    #12 0x6348ff in newsboat::Controller::run(newsboat::CliArgsParser const&) /home/minoru/src/newsboat/src/controller.cpp:483:15
    #13 0x518a06 in main /home/minoru/src/newsboat/newsboat.cpp:185:11
    #14 0x7f1f12bc2bba in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26bba)

SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:147:4 in void __gnu_cxx::new_allocator<unsigned int>::construct<unsigned int, unsigned int const&>(unsigned int*, unsigned int const&)
Shadow bytes around the buggy address:
  0x0c047fffb390: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fa
  0x0c047fffb3a0: fa fa fd fa fa fa fd fd fa fa fd fa fa fa fd fa
  0x0c047fffb3b0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
  0x0c047fffb3c0: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fa
  0x0c047fffb3d0: fa fa fd fa fa fa fd fd fa fa fd fd fa fa fd fa
=>0x0c047fffb3e0: fa[fa]04 fa fa fa 01 fa fa fa fd fd fa fa fd fd
  0x0c047fffb3f0: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fa
  0x0c047fffb400: fa fa fd fa fa fa fd fd fa fa fd fd fa fa fd fd
  0x0c047fffb410: fa fa fd fd fa fa fd fa fa fa fd fa fa fa fd fd
  0x0c047fffb420: fa fa fd fa fa fa fd fa fa fa fd fd fa fa fd fd
  0x0c047fffb430: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fa
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
  Shadow gap:              cc
==28279==ABORTING

[rtyler]

I believe I saw the mention of double free in the gdb output when digging into this. That said, it was a few weeks ago so I wouldn't trust my memory here. I decided to file this ticket because I gave up on trying to have the time to spend on it :smile_cat:

[Minoru]

A user on our mailing list reports the same bug, about this same item. On OpenBSD, it hangs instead of crashing.

[dennisschagt]

Those segmentation faults happen with input like the following.
Article URL: http://blog.netbsd.org/tnf/entry/gsoc_2019_report_adding_netbsd1:

<ol start="1"></ol>
<li>BitFieldDeclarationsOnePerLine</li>
<li>AlignConsecutiveListElements</li>
</ol>

On the first line of that snippet, an ordered list (<ol>) is opened and directly closed (</ol>).
Then at the end it is closed again.

The Newsboat HtmlRenderer adds a value to some vector when encountering an <ol> starting tag and removes a value from that same vector when encountering an </ol> ending tag.
Because of the <ol> tag being closed twice, the vector length gets negative which should never happen.
When returning from the HtmlRenderer::render function, the vector is cleaned up, somehow resulting in a double free (probably undefined behaviour caused by the vector having length == -1).