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...
Actions
Bug #858823
closed
stack-use-after-scope in freeciv-mp-gtk3 (master)
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
Files
Updated by Jacob Nevins over 3 years 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).
- _("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.
Updated by Chippo Elder over 3 years 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.
Updated by Jacob Nevins over 3 years ago
- File m-gui-fcmp-stack-thread-msg.patch m-gui-fcmp-stack-thread-msg.patch added
- File 30-26-gui-fcmp-stack-thread-msg.patch 30-26-gui-fcmp-stack-thread-msg.patch added
- Status changed from New to Resolved
- Assignee set to Jacob Nevins
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
QStringby value. - This article talks about copying vs signals. I think that the
QStringwill 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 *andQStringI usedfromUtf8buttoLocal8Bitwithout 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)
Updated by Jacob Nevins over 3 years 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.
Updated by Jacob Nevins over 3 years ago
- File 30-gtk4-fcmp-stack-thread-msg.patch 30-gtk4-fcmp-stack-thread-msg.patch added
- Status changed from Closed to Resolved
Here's the missing bit.
Updated by Jacob Nevins over 3 years ago
- Status changed from Resolved to Closed
Committed missing bit to S3_0.
Actions
.