Newsboat: Segmentation fault in newsboat::utils::censor_url

[j605]

Newsboat version (copy the output of newsboat -v or the first line of git show):
r2.16.1-2-g909e66

Steps to reproduce the issue:

1. auto-reload yes, no idea if this affects the crash.

2. As the feedlist loads, change the sort order using g to lastupdated.

And it crashes more often than not with the following trace,

#0  0x000055d552a3da3a in newsboat::utils::censor_url (url=<error reading variable: Cannot access memory at address 0xa0>)
    at /usr/include/c++/9.1.0/bits/basic_string.h:2300
#1  0x000055d552a00658 in newsboat::Reloader::reload (this=0x55d55488fda0, pos=17, max=119, unattended=<optimized out>, easyhandle=0x7fdb5effbb28)
    at include/rssfeed.h:117
#2  0x000055d552a01095 in newsboat::Reloader::reload_range (this=0x55d55488fda0, start=<optimized out>, end=<optimized out>, size=119, unattended=false)
    at src/reloader.cpp:280
#3  0x00007fdb68f35cd4 in std::execute_native_thread_routine (__p=0x7fdb58001270) at /build/gcc/src/gcc/libstdc++-v3/src/c++11/thread.cc:80
#4  0x00007fdb6960c57f in start_thread () from /usr/lib/libpthread.so.0
#5  0x00007fdb68c400e3 in clone () from /usr/lib/libc.so.6

[Minoru]

I don't know what exactly is going on, but I think I understand the cause: changing the sort order re-sorts the FeedContainer, while Reloader is spawning threads that look at specific positions in FeedContainer to get feeds. It's hard to imagine what happens after that; I'll wager that a thread running Reloader::reload observes a shared_ptr being exchanged with another shared_ptr; what that Reloader reads is not a valid pointer, and accessing it returns garbage. Then the garbage gets interpreted as std::string, giving us an unlikely pointer of 0xa0, and leading to a crash.

We could keep two separate arrays of feeds. One would be in the same order as the urls file, the other in current feed-sort-order. Reloader will operate on the first one, while FeedListFormAction will use the second. That will fix this particular issue, but will still break if e.g. user edits the urls file—this will change the first array and break Reloader again.

I think the best solution is to move away from indexes in Reloader, and use pointers instead. This will require two changes:

1. Reloader calls FeedContainer::get_all_feeds to get a vector of pointers, splits it into chunks, and passes those chunks to reload_range;
2. Reloader::reload and Controller::replace_feed need to std::move the new feed into the old one. This will keep the old pointer alive and intact, allowing us to sort the feed list however we like.

I'm ready to help and answer further questions if someone wants to work on that.

[Minoru]

As @mjsir911 points out in #791, these two issues look similar. #791 is apparently fixed by #1166 (part of 2.21 release). @j605, can you please build 2.21 and see if this issue still occurs for you?

[j605]

I don't see the crash any more, thank you for fixing it :)