#313 Use-after-free when xfi goes to invalid image file then trying to redraw it

[Mamoru TASAKA]

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=2425678

On LXDE, using fox 1.6.59 and xfe 2.1.2:

• Prepare some valid png file named "valid.png"
• $ mkdir TMP && cd mv valid.png TMP/ && cd TMP
• $ echo "foo" > foo.txt
• $ xfi ./valid.png
• on xfi window, click right arrow button (click "Previous" button)
• popup window with "Unsupported type: txt" appears, click "ok" button on popup window
• now once go to different LXDE workspace
• then go back to the previous workspace (when I launched xfi)

Then xfi crashes with use-after-free:

=================================================================
==564751==ERROR: AddressSanitizer: heap-use-after-free on address 0x7bf8118559a8 at pc 0x7f9813ee2ab8 bp 0x7ffd3bc1ee80 sp 0x7ffd3bc1ee78
READ of size 4 at 0x7bf8118559a8 thread T0
    #0 0x7f9813ee2ab7 in FX::FXDrawable::getWidth() const ../include/FXDrawable.h:62
    #1 0x7f98142638e0 in FX::FXImageView::onPaint(FX::FXObject*, unsigned int, void*) /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXImageView.cpp:143
    #2 0x5587b51a132a in FX::FXApp::dispatchEvent(_XEvent&) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/foxhacks.cpp:1898
    #3 0x7f9813eba074 in FX::FXApp::runOneEvent(bool) /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXApp.cpp:3135
    #4 0x7f9813eba36f in FX::FXApp::run() /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXApp.cpp:3092
    #5 0x5587b4fc90c9 in main /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:3281
    #6 0x7f98130105b4 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #7 0x7f9813010667 in __libc_start_main_impl ../csu/libc-start.c:360
    #8 0x5587b4fc9c54 in _start (/builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/xfi+0x22c54) (BuildId: cc5f1edb83e4cf330de41f095266998df6153f93)

0x7bf8118559a8 is located 40 bytes inside of 64-byte region [0x7bf811855980,0x7bf8118559c0)
freed by thread T0 here:
    #0 0x7f98160e899b in operator delete(void*, unsigned long) (/lib64/libasan.so.8+0xe899b) (BuildId: 5a053b2c39acdde66812d293604438f39b6f73db)
    #1 0x5587b52573c4 in XFileImage::loadimage(FX::FXString const&) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:1639
    #2 0x5587b525ad68 in XFileImage::onCmdItemDoubleClicked(FX::FXObject*, unsigned int, void*) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:2232
    #3 0x7f98143e8d27 in FX::FXObject::tryHandle(FX::FXObject*, unsigned int, void*) /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXObject.cpp:292
    #4 0x5587b50f18da in IconList::onDoubleClicked(FX::FXObject*, unsigned int, void*) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/IconList.cpp:3965
    #5 0x5587b5121a13 in IconList::onKeyPress(FX::FXObject*, unsigned int, void*) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/IconList.cpp:3455
    #6 0x5587b51daa95 in XFileImage::onCmdViewPrev(FX::FXObject*, unsigned int, void*) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:1556
    #7 0x7f98143e8d27 in FX::FXObject::tryHandle(FX::FXObject*, unsigned int, void*) /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXObject.cpp:292
    #8 0x7f9813f09711 in FX::FXButton::onLeftBtnRelease(FX::FXObject*, unsigned int, void*) /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXButton.cpp:258
    #9 0x5587b51a0c7c in FX::FXApp::dispatchEvent(_XEvent&) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/foxhacks.cpp:2270
    #10 0x7f9813eba074 in FX::FXApp::runOneEvent(bool) /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXApp.cpp:3135
    #11 0x7f9813eba36f in FX::FXApp::run() /builddir/fox/rawhide/fox-1.6.59-build/fox-1.6.59/src/FXApp.cpp:3092
    #12 0x5587b4fc90c9 in main /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:3281
    #13 0x7f98130105b4 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #14 0x7f9813010667 in __libc_start_main_impl ../csu/libc-start.c:360
    #15 0x5587b4fc9c54 in _start (/builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/xfi+0x22c54) (BuildId: cc5f1edb83e4cf330de41f095266998df6153f93)

previously allocated by thread T0 here:
    #0 0x7f98160e7a3b in operator new(unsigned long) (/lib64/libasan.so.8+0xe7a3b) (BuildId: 5a053b2c39acdde66812d293604438f39b6f73db)
    #1 0x5587b525923b in XFileImage::loadimage(FX::FXString const&) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:1690
    #2 0x5587b525c24b in XFileImage::start(FX::FXString) /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:2574
    #3 0x5587b4fc909a in main /builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/XFileImage.cpp:3278
    #4 0x7f98130105b4 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #5 0x7f9813010667 in __libc_start_main_impl ../csu/libc-start.c:360
    #6 0x5587b4fc9c54 in _start (/builddir/xfe/rawhide/xfe-2.1.2-build/xfe-2.1.2/src/xfi+0x22c54) (BuildId: cc5f1edb83e4cf330de41f095266998df6153f93)

SUMMARY: AddressSanitizer: heap-use-after-free ../include/FXDrawable.h:62 in FX::FXDrawable::getWidth() const

[Mamoru TASAKA]

Tentative workaround:

diff --git a/src/XFileImage.cpp b/src/XFileImage.cpp
index f5beb1d..14faf4b 100644
--- a/src/XFileImage.cpp
+++ b/src/XFileImage.cpp
@@ -1618,6 +1618,10 @@ FXbool XFileImage::loadimage(const FXString& file)

     FILE* fp = fopen(file.text(), "r");

+    // Save old image
+    FXImage *img_old = img;
+    FXImage *tmpimg_old = tmpimg;
+
     if (!fp)
     {
         MessageBox::error(this, BOX_OK, _("Error"), _("Unable to open file: %s"), file.text());
@@ -1628,18 +1632,6 @@ FXbool XFileImage::loadimage(const FXString& file)
         fclose(fp);
     }

-    // Free old image if any, before loading a new one
-    if (img)
-    {
-        delete img;
-        img = NULL;
-    }
-    if (tmpimg)
-    {
-        delete tmpimg;
-        tmpimg = NULL;
-    }
-
     if (comparecase(ext, "gif") == 0)
     {
         img = new FXGIFImage(getApp(), NULL, IMAGE_KEEP | IMAGE_SHMI | IMAGE_SHMP);
@@ -1701,21 +1693,19 @@ FXbool XFileImage::loadimage(const FXString& file)
     }
     else
     {
-        img = NULL;
-        tmpimg = NULL;
-    }
-
-    // Perhaps failed
-    if (img == NULL)
-    {
+        // Perhaps failed
         MessageBox::error(this, BOX_OK, _("Error Loading Image"), _("Unsupported type: %s"), ext.text());
         return false;
     }

-    if (tmpimg == NULL)
+    // Free old image if any, after loading a new one
+    if (img_old)
     {
-        MessageBox::error(this, BOX_OK, _("Error Loading Image"), _("Unsupported type: %s"), ext.text());
-        return false;
+        delete img_old;
+    }
+    if (tmpimg_old)
+    {
+        delete tmpimg_old;
     }

     // Load it

[Roland Baudin]

Hi, thanks for he bug report. However, I was unable to reproduce the crash in Ubuntu.
I'll investigate further...

[Mamoru TASAKA]

Perhaps recompiling xfe and fox with sanitizer , e.g.

export CC="gcc -fsanitize=address -fsanitize=undefined"
export CXX="g++ -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr"

or using valgrind makes this issue easier to reproduce.

[Roland Baudin]

OK, thanks for the tip!

I tried with valgrind but I saw no error when doing the steps from your bug report.
Then I compiled with your recommended flags and saw no error:

g++ -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -DLOCALEDIR=\"/usr/local/share/locale\" -DHAVE_CONFIG_H -I. -I.. -I. -I.. -I../intl -I/usr/include/freetype2 -I/usr/include/libpng16 -I/usr/include/freetype2 -I/usr/include/libpng16 -O2 -Wall -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -I/usr/include/fox-1.6 -DHAVE_XFT_H -DHAVE_XRANDR_H=1 -DSTARTUP_NOTIFICATION -DXFE_AUTOMOUNTER -MT XFileImage.o -MD -MP -MF $depbase.Tpo -c -o XFileImage.o XFileImage.cpp &&\
mv -f $depbase.Tpo $depbase.Po

g++ -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -O2 -Wall -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr -I/usr/include/fox-1.6 -DHAVE_XFT_H -DHAVE_XRANDR_H=1 -DSTARTUP_NOTIFICATION -DXFE_AUTOMOUNTER -o xfi ../st/x.o ../st/st.o icons.o startupnotification.o xfeutils.o ComboBox.o CommandWindow.o DialogBox.o DirList.o DirHistBox.o File.o FileDialog.o FileDict.o FileList.o HistInputDialog.o IconList.o InputDialog.o MessageBox.o OverwriteBox.o PathLinker.o StringList.o TextLabel.o XFileImage.o ../libsn/sn-common.o ../libsn/sn-launchee.o ../libsn/sn-launcher.o ../libsn/sn-list.o ../libsn/sn-monitor.o ../libsn/sn-util.o ../libsn/sn-xmessages.o ../libsn/sn-xutils.o -lutil -lfontconfig -lpng -lFOX-1.6 -lX11 -lfreetype -lXft -lXrandr -lxcb -lxcb-util -lxcb -lX11-xcb -lX11 -lxcb

I use g++ 13.3.0 in Ubuntu 24.04.

[Mamoru TASAKA]

• on xfi window, click right arrow button (click "Previous" button)

Oh, this is on xfi window, click left arrow button (click "Previous" button), then perhaps popup window with "Unsupported type: txt" appears, sorry...

[Roland Baudin]

No problem, I clicked the left arrow from the beginning. But thanks for the clarification.

[Roland Baudin]

The issue is fixed in the new Xfe 2.1.3. I rewrote some part of the XFileImage code. Thanks for your help in solving this issue.