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.

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

Fix teletext memory overflow #17921

Closed
wants to merge 1 commit into from
Closed

Conversation

repojohnray
Copy link
Contributor

This is a fix for some memory overflow at the teletext level. These issues were visible with valgrind.

@repojohnray
Copy link
Contributor Author

This patch addresses the three following issues below. The logs are generated with Kodi 18.6-Leia:


==28365== Thread 1:
==28365== Invalid read of size 8
==28365==    at 0xBBC887: CTeletextDecoder::RenderPage() (Teletext.cpp:1234)
==28365==    by 0xB863E7: CGUIDialogTeletext::Render() (GUIDialogTeletext.cpp:87)
==28365==    by 0xDE13D4: DoRender (GUIControl.cpp:191)
==28365==    by 0xDE13D4: CGUIControl::DoRender() (GUIControl.cpp:169)
==28365==    by 0xE495AB: CGUIWindow::DoRender() (GUIWindow.cpp:354)
==28365==    by 0xE4DC38: CGUIWindowManager::RenderPass() const (GUIWindowManager.cpp:1154)
==28365==    by 0xE4DED3: CGUIWindowManager::Render() (GUIWindowManager.cpp:1194)
==28365==    by 0xFC9434: Render (Application.cpp:1551)
==28365==    by 0xFC9434: CApplication::Render() (Application.cpp:1515)
==28365==    by 0xE4B364: CGUIWindowManager::ProcessRenderLoop(bool) (GUIWindowManager.cpp:1310)
==28365==    by 0xDF5E46: CGUIDialog::Open_Internal(bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GUIDialog.cpp:201)
==28365==    by 0xDF5FA5: CGUIDialog::Open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GUIDialog.cpp:216)
==28365==    by 0xE50E60: CGUIWindowManager::ActivateWindow_Internal(int, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool, bool) (GUIWindowManager.cpp:816)
==28365==    by 0xE512DD: CGUIWindowManager::ActivateWindow(int, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool, bool) (GUIWindowManager.cpp:764)
==28365==  Address 0x2798fce0 is 0 bytes inside a block of size 35 free'd
==28365==    at 0x4839022: operator delete(void*) (in /usr/local/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==28365==    by 0xEB5008: deallocate (new_allocator.h:125)
==28365==    by 0xEB5008: deallocate (alloc_traits.h:462)
==28365==    by 0xEB5008: _M_destroy (basic_string.h:226)
==28365==    by 0xEB5008: _M_dispose (basic_string.h:221)
==28365==    by 0xEB5008: ~basic_string (basic_string.h:657)
==28365==    by 0xEB5008: dbiplus::field_value::field_value(dbiplus::field_value const&) (qry_dat.cpp:102)
==28365==    by 0xEB2C8D: dbiplus::Dataset::get_field_value(int) (dataset.cpp:390)
==28365==    by 0xEB2AB7: dbiplus::Dataset::get_field_value(char const*) (dataset.cpp:358)
==28365==    by 0x1132A10: fv (dataset.h:376)
==28365==    by 0x1132A10: PVR::CPVREpgDatabase::Get(PVR::CPVREpg const&) (EpgDatabase.cpp:264)
==28365==    by 0x112E416: PVR::CPVREpg::Load(std::shared_ptr<PVR::CPVREpgDatabase> const&) (Epg.cpp:280)
==28365==    by 0x112777B: PVR::CPVREpgContainer::LoadFromDB() (EpgContainer.cpp:287)
==28365==    by 0x1127ABA: PVR::CPVREpgContainer::Start(bool) (EpgContainer.cpp:202)
==28365==    by 0x112B8FA: PVR::CPVREpgContainerStartJob::DoWork() (EpgContainer.cpp:175)
==28365==    by 0xCC0A20: CJobWorker::Process() (JobManager.cpp:55)
==28365==    by 0xD2B4A1: CThread::Action() (Thread.cpp:208)
==28365==    by 0xD2C22C: CThread::staticThread(void*) (Thread.cpp:116)
==28365==  Block was alloc'd at
==28365==    at 0x4837E62: operator new(unsigned long) (in /usr/local/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==28365==    by 0x9102D8E: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (in /usr/lib64/libstdc++.so.6.0.25)
==28365==    by 0xEB4CDB: assign (basic_string.h:1366)
==28365==    by 0xEB4CDB: operator= (basic_string.h:695)
==28365==    by 0xEB4CDB: dbiplus::field_value::get_asString[abi:cxx11]() const (qry_dat.cpp:157)
==28365==    by 0xEB4FEB: dbiplus::field_value::field_value(dbiplus::field_value const&) (qry_dat.cpp:102)
==28365==    by 0xEB2C8D: dbiplus::Dataset::get_field_value(int) (dataset.cpp:390)
==28365==    by 0xEB2AB7: dbiplus::Dataset::get_field_value(char const*) (dataset.cpp:358)
==28365==    by 0x1132A10: fv (dataset.h:376)
==28365==    by 0x1132A10: PVR::CPVREpgDatabase::Get(PVR::CPVREpg const&) (EpgDatabase.cpp:264)
==28365==    by 0x112E416: PVR::CPVREpg::Load(std::shared_ptr<PVR::CPVREpgDatabase> const&) (Epg.cpp:280)
==28365==    by 0x112777B: PVR::CPVREpgContainer::LoadFromDB() (EpgContainer.cpp:287)
==28365==    by 0x1127ABA: PVR::CPVREpgContainer::Start(bool) (EpgContainer.cpp:202)
==28365==    by 0x112B8FA: PVR::CPVREpgContainerStartJob::DoWork() (EpgContainer.cpp:175)
==28365==    by 0xCC0A20: CJobWorker::Process() (JobManager.cpp:55)



==28365== Invalid read of size 1
==28365==    at 0xBBA923: CTeletextDecoder::DoFlashing(int) (Teletext.cpp:1307)
==28365==    by 0xBBC66C: CTeletextDecoder::RenderPage() (Teletext.cpp:1298)
==28365==    by 0xB863E7: CGUIDialogTeletext::Render() (GUIDialogTeletext.cpp:87)
==28365==    by 0xDE13D4: DoRender (GUIControl.cpp:191)
==28365==    by 0xDE13D4: CGUIControl::DoRender() (GUIControl.cpp:169)
==28365==    by 0xE495AB: CGUIWindow::DoRender() (GUIWindow.cpp:354)
==28365==    by 0xE4DC38: CGUIWindowManager::RenderPass() const (GUIWindowManager.cpp:1154)
==28365==    by 0xE4DED3: CGUIWindowManager::Render() (GUIWindowManager.cpp:1194)
==28365==    by 0xFC9434: Render (Application.cpp:1551)
==28365==    by 0xFC9434: CApplication::Render() (Application.cpp:1515)
==28365==    by 0xE4B364: CGUIWindowManager::ProcessRenderLoop(bool) (GUIWindowManager.cpp:1310)
==28365==    by 0xDF5E46: CGUIDialog::Open_Internal(bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GUIDialog.cpp:201)
==28365==    by 0xDF5FA5: CGUIDialog::Open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GUIDialog.cpp:216)
==28365==    by 0xE50E60: CGUIWindowManager::ActivateWindow_Internal(int, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool, bool) (GUIWindowManager.cpp:816)
==28365==  Address 0x33c03ca0 is 16 bytes inside a block of size 968 free'd
==28365==    at 0x4839022: operator delete(void*) (in /usr/local/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==28365==    by 0xB48338: CDVDTeletextData::ResetTeletextCache() (VideoPlayerTeletext.cpp:166)
==28365==    by 0xB48E1B: CDVDTeletextData::Process() (VideoPlayerTeletext.cpp:632)
==28365==    by 0xD2B4A1: CThread::Action() (Thread.cpp:208)
==28365==    by 0xD2C22C: CThread::staticThread(void*) (Thread.cpp:116)
==28365==    by 0x486BF15: start_thread (in /lib64/libpthread-2.31.so)
==28365==    by 0x8F16B4E: clone (in /lib64/libc-2.31.so)
==28365==  Block was alloc'd at
==28365==    at 0x4837E62: operator new(unsigned long) (in /usr/local/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==28365==    by 0xB48A17: CDVDTeletextData::AllocateCache(int) (VideoPlayerTeletext.cpp:750)
==28365==    by 0xB494A8: CDVDTeletextData::Process() (VideoPlayerTeletext.cpp:334)
==28365==    by 0xD2B4A1: CThread::Action() (Thread.cpp:208)
==28365==    by 0xD2C22C: CThread::staticThread(void*) (Thread.cpp:116)
==28365==    by 0x486BF15: start_thread (in /lib64/libpthread-2.31.so)
==28365==    by 0x8F16B4E: clone (in /lib64/libc-2.31.so)



==28365== Thread 1:
==28365== Invalid read of size 1
==28365==    at 0xBBA7A5: CTeletextDecoder::RenderCharIntern(TextRenderInfo_t*, int, TextPageAttr_t*, int, int) (Teletext.cpp:2255)
==28365==    by 0xBBC31C: CTeletextDecoder::DoRenderPage(int, int) (Teletext.cpp:1502)
==28365==    by 0xB863E7: CGUIDialogTeletext::Render() (GUIDialogTeletext.cpp:87)
==28365==    by 0xDE13D4: DoRender (GUIControl.cpp:191)
==28365==    by 0xDE13D4: CGUIControl::DoRender() (GUIControl.cpp:169)
==28365==    by 0xE495AB: CGUIWindow::DoRender() (GUIWindow.cpp:354)
==28365==    by 0xE4DC38: CGUIWindowManager::RenderPass() const (GUIWindowManager.cpp:1154)
==28365==    by 0xE4DED3: CGUIWindowManager::Render() (GUIWindowManager.cpp:1194)
==28365==    by 0xFC9434: Render (Application.cpp:1551)
==28365==    by 0xFC9434: CApplication::Render() (Application.cpp:1515)
==28365==    by 0xE4B364: CGUIWindowManager::ProcessRenderLoop(bool) (GUIWindowManager.cpp:1310)
==28365==    by 0xDF5E46: CGUIDialog::Open_Internal(bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GUIDialog.cpp:201)
==28365==    by 0xDF5FA5: CGUIDialog::Open(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (GUIDialog.cpp:216)
==28365==    by 0xE50E60: CGUIWindowManager::ActivateWindow_Internal(int, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool, bool) (GUIWindowManager.cpp:816)
==28365==  Address 0x303b23f8 is 2 bytes after a block of size 6 alloc'd
==28365==    at 0x4837792: malloc (in /usr/local/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==28365==    by 0x79EE733: ??? (in /usr/X11R6/lib64/libfreetype.so.6.17.2)
==28365==    by 0x7A4154E: ??? (in /usr/X11R6/lib64/libfreetype.so.6.17.2)
==28365==    by 0x7A4168C: ??? (in /usr/X11R6/lib64/libfreetype.so.6.17.2)
==28365==    by 0x7A42076: FTC_SBitCache_Lookup (in /usr/X11R6/lib64/libfreetype.so.6.17.2)
==28365==    by 0xBBA710: CTeletextDecoder::RenderCharIntern(TextRenderInfo_t*, int, TextPageAttr_t*, int, int) (Teletext.cpp:2245)
==28365==    by 0xBBC31C: CTeletextDecoder::DoRenderPage(int, int) (Teletext.cpp:1502)
==28365==    by 0xB863E7: CGUIDialogTeletext::Render() (GUIDialogTeletext.cpp:87)
==28365==    by 0xDE13D4: DoRender (GUIControl.cpp:191)
==28365==    by 0xDE13D4: CGUIControl::DoRender() (GUIControl.cpp:169)
==28365==    by 0xE495AB: CGUIWindow::DoRender() (GUIWindow.cpp:354)
==28365==    by 0xE4DC38: CGUIWindowManager::RenderPass() const (GUIWindowManager.cpp:1154)
==28365==    by 0xE4DED3: CGUIWindowManager::Render() (GUIWindowManager.cpp:1194)


@fuzzard fuzzard added Type: Fix non-breaking change which fixes an issue v19 Matrix labels May 22, 2020
@fuzzard fuzzard added this to the Matrix 19.0-alpha 1 milestone May 22, 2020
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the current code guidelines.

xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
@repojohnray
Copy link
Contributor Author

Updated.

Copy link
Member

@AlwinEsch AlwinEsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to start improve of this. Except them memory place it is OK for me.

xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
@repojohnray
Copy link
Contributor Author

repojohnray commented May 25, 2020

__GNUC__ is defined by gcc and llvm; This is likely the best option to handle all compilers.

xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
@DaveTBlake
Copy link
Member

As a fix mived to Beta1 milestone. However changes are requested @repojohnray, are you able to do this or should this be deferred to v20

@repojohnray
Copy link
Contributor Author

repojohnray commented Jul 6, 2022

Updated - This commit is compatible with Nexus. The array is set to 8k and the dynamic version of this line is compatible with gcc and clang.

@garbear
Copy link
Member

garbear commented Jul 6, 2022

Why have a dynamic version for GCC? Surely the static version is more memory-friendly by avoiding the heap? Also we probably don't want behavior to change if someone just flips in a different compiler.

@repojohnray
Copy link
Contributor Author

repojohnray commented Jul 7, 2022

Why have a dynamic version for GCC? Surely the static version is more memory-friendly by avoiding the heap? Also we probably don't want behavior to change if someone just flips in a different compiler.

The original code has this length hardcoded, I have not tried to rewrite the code to avoid this issue. Using variable length array will avoid any memory overflow for future screen resolutions; LLVM is defining GNUC as well. But, you are right, this issue could happen again with compilers incompatible with VLA.

@repojohnray
Copy link
Contributor Author

Hi @jenkins4kodi, the commit is updated.

xbmc/video/Teletext.cpp Outdated Show resolved Hide resolved
@enen92
Copy link
Member

enen92 commented Jul 15, 2022

jenkins build this please

@enen92 enen92 requested a review from AlwinEsch July 16, 2022 16:57
@@ -2255,7 +2262,11 @@ void CTeletextDecoder::RenderCharIntern(TextRenderInfo_t* RenderInfo, int Char,

/* render char */
sbitbuffer = m_sBit->buffer;
unsigned char localbuffer[1000]; // should be enough to store one character-bitmap...

std::vector<unsigned char> localbuffer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how this part works (teletext) but I think it's very inefficient to create/destroy a vector to render just one character. I suspect that this method will be called multiple times in a row.

It's much better to just increase the buffer size to 2000, 3000, 5000, 10000 or whatever is needed and keep it on the stack. How many pixels does a teletext character have?

The rest of the changes looks fine.

@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Dec 5, 2022
@jenkins4kodi
Copy link
Contributor

@repojohnray this needs a rebase

@repojohnray repojohnray closed this Dec 6, 2022
@enen92
Copy link
Member

enen92 commented Dec 6, 2022

@repojohnray issue did disappear with the last merged change or did you simply gave up on this?

@repojohnray repojohnray mentioned this pull request Dec 7, 2022
13 tasks
@repojohnray
Copy link
Contributor Author

@enen92, I have opened a new PR that should close this issue: #22226

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Rebase needed PR that does not apply/merge cleanly to current base branch Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants