Closed xaxxon closed 8 years ago
Those are two different things you are discussing.
The first one is an assert that checks that you didn't alter the text buffer in the text edit custom callback without maintaining its length. Have you read the comment next to it?
// You need to maintain BufTextLen if you change the text!
IM_ASSERT(callback_data.BufTextLen == (int)strlen(callback_data.Buf));
The second, can you double-check that i->message.c_str()
point to a proper zero-terminated string?
If you look at the string passed to TextUnformatted() is looks rather dodgy, it is mean to be that long (multiple lines of content)?
Both appears to be bug in your code not in imgui..
That's an example of what it looks like without line wrapping.
Yes, I realized after I posted the edit that the two were unrelated and removed the edit. I'll do those other things now.
I wonder if ImGui::TextV()
behave correctly on overflow? Check around that.
Can you add test/breakpoint to see if the w==-1
codepath got executed?
int ImFormatStringV(char* buf, int buf_size, const char* fmt, va_list args)
{
int w = vsnprintf(buf, buf_size, fmt, args);
buf[buf_size-1] = 0;
return (w == -1) ? buf_size : w;
}
PS: If you are going to display big chunk of text here using TextUnformatted(buf)
will be faster than Text("%s", buf);
and not have a size limitation. Bug if any should definitively be fixed.
**frame #7: 0x0000000106c64f9c apb`ImGui::TextUnformatted(**text="Module: core Callback new_map failed: Couldn't find object factory named core/fueltruck2\n0 apb 0x000000010079911a _ZN19StackTraceExceptionC2Ev + 506\n1 apb 0x000000010010777c _ZN12ApbExceptionC2IJRKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEEEEES9_DpOT_ + 284\n2 apb 0x00000001000faf85 _ZN12ApbExceptionC1IJRKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEEEEES9_DpOT_ + 37\n3 apb 0x000000010059f97f _ZNK16ObjectCollection3getERKNSt3__112basic_stringIcNS0_11char_traitsIcEENS0_9allocatorIcEEEE + 2783\n4 apb 0x0000000100bcbac0 _ZN9v8toolkit4BindI7JobListMS1_FvR3JobEEclES3_ + 320\n5 apb 0x0000000100bcb96d _ZNSt3__128__invoke_void_return_wrapperIvE6__callIJRN9v8toolkit4BindI7JobListMS5_FvR3JobEEES7_EEEvDpOT_ + 77\n6 apb 0x0000000100bcb7c9 _ZNSt3__110__fu"..., text_end="") + 5580 at imgui.cpp:5394
5391 else
5392 {
5393 const float wrap_width = wrap_enabled ? CalcWrapWidthForPos(window->DC.CursorPos, wrap_pos_x) : 0.0f;
-> 5394 const ImVec2 text_size = CalcTextSize(text_begin, text_end, false, wrap_width);
5395
5396 // Account of baseline offset
5397 ImVec2 text_pos(window->DC.CursorPos.x, window->DC.CursorPos.y + window->DC.CurrentLineTextBaseOffset);
(lldb) p text_begin;
(const char *) $1 = 0x00000001073ff878 "Module: core Callback new_map failed: Couldn't find object factory named core/fueltruck2\n0 apb 0x000000010079911a _ZN19StackTraceExceptionC2Ev + 506\n1 apb 0x000000010010777c _ZN12ApbExceptionC2IJRKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEEEEES9_DpOT_ + 284\n2 apb 0x00000001000faf85 _ZN12ApbExceptionC1IJRKNSt3__112basic_stringIcNS1_11char_traitsIcEENS1_9allocatorIcEEEEEEES9_DpOT_ + 37\n3 apb 0x000000010059f97f _ZNK16ObjectCollection3getERKNSt3__112basic_stringIcNS0_11char_traitsIcEENS0_9allocatorIcEEEE + 2783\n4 apb 0x0000000100bcbac0 _ZN9v8toolkit4BindI7JobListMS1_FvR3JobEEclES3_ + 320\n5 apb 0x0000000100bcb96d _ZNSt3__128__invoke_void_return_wrapperIvE6__callIJRN9v8toolkit4BindI7JobListMS5_FvR3JobEEES7_EEEvDpOT_ + 77\n6 apb 0x0000000100bcb7c9 _ZNSt3__110__fu"...
(lldb) p text_end
(const char *) $2 = 0x00000001074006f1 ""
and
frame #11: 0x00000001001e4ce2 apb`Console::draw(this=0x00007fff5fbfdb68)::$_3::operator()() const + 10114 at console.cpp:218
215 auto len = strlen(i->message.c_str());
216 fprintf(stderr, "strlen of thing I'm sending to imgui: %lu\n", len);
217
-> 218 ImGui::TextColored(ImVec4(1, .2, .2, 1), "%s", i->message.c_str());
219 ImGui::PopTextWrapPos();
220 break;
221 }
(lldb)
outputs:
Generating framebuffers with dimensions 1024 768
Destroying framebuffers - happens when resizing window and closing program
Setting timer delta to 0 because time difference between updates > 3s
**strlen of thing I'm sending to imgui: 3705**
=================================================================
==17219==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000107400480 at pc 0x000106d6115e bp 0x7fff5fbfb970 sp 0x7fff5fbfb968
READ of size 1 at 0x000107400480 thread T0
#0 0x106d6115d in ImFont::CalcTextSizeA(float, float, float, char const*, char const*, char const**) const (/Users/xaxxon/Library/Caches/CLion2016.2/cmake/generated/apb-21357782/21357782/Debug/apb+0x106d6115d)
#1 0x106c6bfc2 in ImGui::CalcTextSize(char const*, char const*, bool, float) (/Users/xaxxon/Library/Caches/CLion2016.2/cmake/generated/apb-21357782/21357782/Debug/apb+0x106c6bfc2)
#2 0x106c64f9b in ImGui::TextUnformatted(char const*, char const*) (/Users/xaxxon/Library/Caches/CLion2016.2/cmake/generated/apb-21357782/21357782/Debug/apb+0x106c64f9b)
OK so text_end
is very wrong here. Could you track what happens in TextV() ?
OK in ImFormatString
and ImFormatStringV
Try to change
return (w == -1) ? buf_size-1 : w;
to
return (w == -1) ? buf_size : w;
Assuming your lib/compiler version of vsnprintf if one that ever returns -1. Or inspect around that.
IGNORE THIS:
I put a breakpoint in ImFormatStringV and it is not being called for the string that crashes. I get a bunch that work, but the crash happens (it's a clang address santizier check fail, technically) before the breakpoint is hit. Address sanitization CAN give false positives...
It's crashing before it gets to TextV, as well.
I'm on clang 3.8 on os x.
This is odd -- I send the string "no jobs" through, and it works. Then I send the "keyboard mouse text" line through and it works. Then I send the really long one through, the "strlen of thing I'm sending to imgui: 3706" is expected, but then the next breakpoint says the string is "no jobs" but the length is 3706. And then it crashes (I trimmed the crash report, but you can see it beginning)
Process 17282 resuming
Process 17282 stopped
* thread #1: tid = 0x89c4c, 0x0000000106ca6890 apb`ImGui::TextV(fmt="No jobs", args=0x00007fff5fbfd200) + 16 at imgui.cpp:5238, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
frame #0: 0x0000000106ca6890 apb`ImGui::TextV(fmt="No jobs", args=0x00007fff5fbfd200) + 16 at imgui.cpp:5238
5235
5236 void ImGui::TextV(const char* fmt, va_list args)
5237 {
-> 5238 ImGuiWindow* window = GetCurrentWindow();
5239 if (window->SkipItems)
5240 return;
5241
(lldb) c
Process 17282 resuming
Process 17282 stopped
* thread #1: tid = 0x89c4c, 0x0000000106c429e7 apb`ImFormatStringV(buf="keyboard: 0 mouse: 0 text: 0", buf_size=3073, fmt="No jobs", args=0x00007fff5fbfd200) + 23 at imgui.cpp:959, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
frame #0: 0x0000000106c429e7 apb`ImFormatStringV(buf="keyboard: 0 mouse: 0 text: 0", buf_size=3073, fmt="No jobs", args=0x00007fff5fbfd200) + 23 at imgui.cpp:959
956
957 int ImFormatStringV(char* buf, int buf_size, const char* fmt, va_list args)
958 {
-> 959 int w = vsnprintf(buf, buf_size, fmt, args);
960 buf[buf_size-1] = 0;
961 return (w == -1) ? buf_size : w;
962 }
(lldb)
Process 17282 resuming
strlen of thing I'm sending to imgui: 3706
Process 17282 stopped
* thread #1: tid = 0x89c4c, 0x0000000106ca6890 apb`ImGui::TextV(fmt="%s", args=0x00007fff5fbfc5c0) + 16 at imgui.cpp:5238, queue = 'com.apple.main-thread', stop reason = breakpoint 3.1
frame #0: 0x0000000106ca6890 apb`ImGui::TextV(fmt="%s", args=0x00007fff5fbfc5c0) + 16 at imgui.cpp:5238
5235
5236 void ImGui::TextV(const char* fmt, va_list args)
5237 {
-> 5238 ImGuiWindow* window = GetCurrentWindow();
5239 if (window->SkipItems)
5240 return;
5241
(lldb) p args
(__va_list_tag *) $11 = 0x00007fff5fbfc5c0
(lldb) c
Process 17282 resuming
Process 17282 stopped
* thread #1: tid = 0x89c4c, 0x0000000106c429e7 apb`ImFormatStringV(buf="No jobs", buf_size=3073, fmt="%s", args=0x00007fff5fbfc5c0) + 23 at imgui.cpp:959, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
frame #0: 0x0000000106c429e7 apb`ImFormatStringV(buf="No jobs", buf_size=3073, fmt="%s", args=0x00007fff5fbfc5c0) + 23 at imgui.cpp:959
956
957 int ImFormatStringV(char* buf, int buf_size, const char* fmt, va_list args)
958 {
-> 959 int w = vsnprintf(buf, buf_size, fmt, args);
960 buf[buf_size-1] = 0;
961 return (w == -1) ? buf_size : w;
962 }
(lldb) c
Process 17282 resuming
=================================================================
==17282==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000107400480 at pc 0x000106d6115e bp 0x7fff5fbfb970 sp 0x7fff5fbfb968
READ of size 1 at 0x000107400480 thread T0
#0 0x106d6115d in ImFont::CalcTextSizeA(float, float, float, char const*, char const*, char const**) const (/Users/xaxxon/Library/Caches/CLion2016.2/cmake/generated/apb-21357782/21357782/Debug/apb+0x106d6115d)
#1 0x106c6bfc2 in ImGui::CalcTextSize(char const*, char const*, bool, float) (/Users/xaxxon/Library/Caches/CLion2016.2/cmake/generated/apb-21357782/21357782/Debug/apb+0x106c6bfc2)
You said:
Try to change
return (w == -1) ? buf_size-1 : w; to return (w == -1) ? buf_size : w;
but it already says that:
Sorry the other way around, add the -1
Not sure if you had already figured this out for sure, but it works for TextUnformatted:
ImGui::PushTextWrapPos(0.0f);
ImGui::TextUnformatted(i->message.c_str());
ImGui::PopTextWrapPos();
adding the -1 now
Yes, but it must be fixed for the other path. Does the -1 fixes it?
I'm finding it hard to read or grasp the explanation or content of most of your posts above..
e.g.
This is odd -- I send the string "no jobs" through, and it works. Then I send the "keyboard mouse text" line through and it works. Then I send the really long one through, the "strlen of thing I'm sending to imgui: 3706" is expected, but then the next breakpoint says the string is "no jobs" but the length is 3706.
adding the -1 in those two places didn't seem to change anything.
int ImFormatString(char* buf, int buf_size, const char* fmt, ...)
{
va_list args;
va_start(args, fmt);
int w = vsnprintf(buf, buf_size, fmt, args);
va_end(args);
buf[buf_size-1] = 0;
return (w == -1) ? buf_size-1 : w;
}
int ImFormatStringV(char* buf, int buf_size, const char* fmt, va_list args)
{
int w = vsnprintf(buf, buf_size, fmt, args);
buf[buf_size-1] = 0;
return (w == -1) ? buf_size-1 : w;
}
Hmm... if you have a predictable repro, could you trace into it and figure out at which point text_end
becomes miscalculated?
With this code-path it will use GImGui->TempBuffer
as a temporary buffer within TextV.
char TempBuffer[1024*3+1]; // temporary text buffer
3073 bytes,
So GImGui->TempBuffer[3072] is valid, GImGui->TempBuffer[3073] not valid. Since the buffer is limited your text will be clamped here and text_end should be == GImGui->TempBuffer+3072
I can't find to find a repro so far. :(
The -1 fix is most definitively a correct fix that needs to be pushed, I'm very surprised this doesn't fix it for you.
You're building with clang and -fsanitize=address set?
I'm building with visuals and adding asserts.
This probably isn't the most useful format, but this is a decimal ascii dump of the string. I have to go to sleep for the hour before I need to wake up, and I'll look at this more this afternoon/evening pacific time. Thanks for all your help. I really hope this isn't some sort of weird clang address sanitization bug...
[deleted ridiculousness]
err, how about fopen("dump.txt", "wb") fwrite() fclose()?
So when I'm looking at it later, what is text_end
supposed to be?
Hrmm, I wonder if trailing \n's cause problems?
OK I think I know what it is, your vsnprintf() behave differently than MSVCRT one.
The snprintf() and vsnprintf() functions will write at most size-1 of the characters printed into the output string (the size'th char- acter then gets the terminating `\0'); if the return value is greater than or equal to the size argument, the string was too short and some of the printed characters were discarded. The output is always null-terminated, unless size is 0.
C libraries can't agree on a behavior for vsnprintf(), great..
try:
int ImFormatString(char* buf, int buf_size, const char* fmt, ...)
{
va_list args;
va_start(args, fmt);
int w = vsnprintf(buf, buf_size, fmt, args);
va_end(args);
if (w == -1 || w >= buf_size)
w = buf_size - 1;
buf[w] = 0;
return w;
}
int ImFormatStringV(char* buf, int buf_size, const char* fmt, va_list args)
{
int w = vsnprintf(buf, buf_size, fmt, args);
if (w == -1 || w >= buf_size)
w = buf_size - 1;
buf[w] = 0;
return w;
}
I think we can safely conclude that you are, in fact, da man.
It is mildly annoying because glibc and msvcrt behave differently, and there's actually genuine use for both behaviors. Right now with the way we are using it with the glibc behavior it is doing a little unnecessary work (we wouldn't mind if it stopped counting).
I'll push a fix. In your situation you should still use TextUnformatted
either way :)
Thanks for the help. Safe sleep!
http://demin.ws/blog/english/2013/01/28/use-snprintf-on-different-platforms/
Yikes:
On Windows the situation is even worse. First, the function returns -1 similar to HP-UX if the buffer is not big enough. Second, it does not count the trailing zero in the length of its output, which means it does not append the trailing zero at all in the situation of the small buffer. Though, Microsoft does not recommend using snprintf() at all as a non-thread safe function, and recommends _snprintf_s() instead.
I had some code that printed text in a window and it was sometimes too long, so I thought I'd change it to use line wrapping. I tried changing from text colored to text wrapped, and it crashed. So I saw that in the header file it said text wrapped was just a helper around a couple other calls, so I added those other calls aroudn my textcolored instead. Still crashed. I then upgraded to latest version, and it still crashes.
In the very bottom of this listing, you can see my code -- I pointed out which line I had before that worked and the line directly above and below it that are causing the crash.
Happy to provide any more data.