Project

General

Profile

Bug #858823

stack-use-after-scope in freeciv-mp-gtk3 (master)

Added by Chippo Elder 5 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Modpack Installer
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

I started the modpack installer and clicked on the top thing and then clicked Install modpack, and got this:

chippo@chippo-Aspire-V3-731:~$ freeciv-mp-gtk3-dev
3: Installing modpack amplio.mpdl from http://files.freeciv.org/modinst/dev/amplio.mpdl
=================================================================
==454751==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fea5bf40c80 at pc 0x000000434ab9 bp 0x7ffde850d640 sp 0x7ffde850ce00
READ of size 40 at 0x7fea5bf40c80 thread T0
    #0 0x434ab8 in strlen (/home/chippo/bin/freeciv-mp-gtk3-dev+0x434ab8)
    #1 0x7fea65f37747 in g_strdup (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x71747)
    #2 0x7fea66747fb9 in gtk_label_set_text (/usr/lib/x86_64-linux-gnu/libgtk-3.so.0+0x239fb9)
    #3 0x4ca586 in msg_callback /home/chippo/Downloads/git_clones/freeciv/freeciv-dev/tools/fcmp/mpgui_gtk3.c:135:3
    #4 0x4caac0 in msg_main_thread /home/chippo/Downloads/git_clones/freeciv/freeciv-dev/tools/fcmp/mpgui_gtk3.c:160:3
    #5 0x7fea6643aeec  (/usr/lib/x86_64-linux-gnu/libgdk-3.so.0+0x30eec)
    #6 0x7fea65f1771d in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5171d)
    #7 0x7fea65f17acf  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51acf)
    #8 0x7fea65f17dc2 in g_main_loop_run (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51dc2)
    #9 0x7fea6675bc2c in gtk_main (/usr/lib/x86_64-linux-gnu/libgtk-3.so.0+0x24dc2c)
    #10 0x4c95bf in main /home/chippo/Downloads/git_clones/freeciv/freeciv-dev/tools/fcmp/mpgui_gtk3.c:627:5
    #11 0x7fea65a971e2 in __libc_start_main /build/glibc-4WA41p/glibc-2.30/csu/../csu/libc-start.c:308:16
    #12 0x421c6d in _start (/home/chippo/bin/freeciv-mp-gtk3-dev+0x421c6d)

Address 0x7fea5bf40c80 is located in stack of thread T6 (Downloader) at offset 8736 in frame
    #0 0x4caebf in download_modpack_recursive /home/chippo/Downloads/git_clones/freeciv/freeciv-dev/tools/fcmp/download.c:82

  This frame has 9 object(s):
    [32, 2080) 'local_dir' (line 83)
    [2208, 4256) 'local_name' (line 84)
    [4384, 6432) 'baseURL' (line 94)
    [6560, 8608) 'fileURL' (line 95)
    [8736, 10784) 'buf' (line 128) <== Memory access at offset 8736 is inside this variable
    [10912, 12960) 'dep_URL_full' (line 232)
    [13088, 15136) 'buf252' (line 310)
    [15264, 17312) 'buf293' (line 353)
    [17440, 19488) 'buf309' (line 363)
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
Thread T6 (Downloader) created by T0 here:
    #0 0x4842fa in pthread_create (/home/chippo/bin/freeciv-mp-gtk3-dev+0x4842fa)
    #1 0x7fea65f63a16  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x9da16)

SUMMARY: AddressSanitizer: stack-use-after-scope (/home/chippo/bin/freeciv-mp-gtk3-dev+0x434ab8) in strlen
Shadow bytes around the buggy address:
  0x0ffdcb7e0140: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffdcb7e0150: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffdcb7e0160: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffdcb7e0170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ffdcb7e0180: f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2
=>0x0ffdcb7e0190:[f8]f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x0ffdcb7e01a0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x0ffdcb7e01b0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x0ffdcb7e01c0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x0ffdcb7e01d0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
  0x0ffdcb7e01e0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
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
==454751==ABORTING
m-gui-fcmp-stack-thread-msg.patch (4.26 KB) m-gui-fcmp-stack-thread-msg.patch master (Gtk4 not tested) Jacob Nevins, 2020-02-09 02:42 PM
30-26-gui-fcmp-stack-thread-msg.patch (4.18 KB) 30-26-gui-fcmp-stack-thread-msg.patch S3_0, S2_6 Jacob Nevins, 2020-02-09 02:42 PM
30-gtk4-fcmp-stack-thread-msg.patch (1019 Bytes) 30-gtk4-fcmp-stack-thread-msg.patch S3_0: missing Gtk4 fix (untested but trivial backport from master) Jacob Nevins, 2020-02-11 09:07 PM

History

#1 Updated by Jacob Nevins 5 months ago

  • Category set to Modpack Installer
  • Target version set to 2.6.2

Not tried to reproduce, but I'm guessing from code inspection that this is because download_modpack_recursive() calls its dl_msg_callback callback with a string on its stack; but in mpgui_gtk3 the callback is msg_dl_thread() which stashes that (stack) pointer in a msg_data and passes that to a thread.

Probably msg_dl_thread() should strdup its msg (and that copy should later be freed).

Examples of messages in stack buffers:
  • _("Downloading \"%s\" control file.")
  • _("Illegal path for %s")
  • "Downloading %s"
  • "Failed to download %s"
  • Callee netfile_get_section_file() can do the same, e.g. _("Failed to fetch %s: %s")

I think other modpack installers and the 2.6 branch have the same problem.

#2 Updated by Chippo Elder 5 months ago

Jacob Nevins wrote:

I think other modpack installers and the 2.6 branch have the same problem.

I've tried to reproduce in S2_6 and haven't succeeded yet.

#3 Updated by Jacob Nevins 5 months ago

I've reproduced trouble with valgrind (including on S2_6), and this resolves it for me for all GUIs (Gtk2/3/4, Qt).

I'm quite unfamiliar with the Qt/C++ memory and string management, so I'd appreciate any review of the mpgui-qt stuff. I think it's correct:
  • This is the first instance of sending a mutable string through a signal that I found in our codebase on a quick look. I chose to pass a QString by value.
  • This article talks about copying vs signals. I think that the QString will have various copies, including different ones in different threads, which each get finalised at appropriate times; and the underlying string data is shared with thread-safe reference counting, so should get freed at the right time (when the receiving thread/slot is done with it).
  • I was careful not to exacerbate bug #859139.
  • To go between const char * and QString I used fromUtf8 but toLocal8Bit without thinking about it much, since both seem to be existing practice. Maybe that is dubious. (I tested non-English languages but everything is UTF-8 on my system)

#4 Updated by Jacob Nevins 5 months ago

  • Status changed from Resolved to Closed

#5 Updated by Marko Lindqvist 5 months ago

S3_0 gui_gtk4.c was not changed?

#6 Updated by Jacob Nevins 5 months ago

Aiee, I hadn't even spotted that it exists. In my mind Gtk4 is a 3.1+ thing (because that's true of the main client), and I developed the patch on S2_6 and merged toward master.

#7 Updated by Jacob Nevins 5 months ago

Here's the missing bit.

#8 Updated by Jacob Nevins 5 months ago

  • Status changed from Resolved to Closed

Committed missing bit to S3_0.

Also available in: Atom PDF