Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #858823

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

Added by Chippo Elder about 1 year ago. Updated about 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Modpack Installer
Sprint/Milestone:
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 about 1 year ago

  • Category set to Modpack Installer
  • Sprint/Milestone 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 about 1 year 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 about 1 year 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 about 1 year ago

  • Status changed from Resolved to Closed

#5 Updated by Marko Lindqvist about 1 year ago

S3_0 gui_gtk4.c was not changed?

#6 Updated by Jacob Nevins about 1 year 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 about 1 year ago

Here's the missing bit.

#8 Updated by Jacob Nevins about 1 year ago

  • Status changed from Resolved to Closed

Committed missing bit to S3_0.

Also available in: Atom PDF