Crash on tooltip hovering: snprintf buffer overflow fix#350
Crash on tooltip hovering: snprintf buffer overflow fix#350caco3 wants to merge 1 commit intozevv:v1.5.0-rc2from
Conversation
The gen_tooltip function now properly tracks the remaining buffer space: Added bounds checking with if(r > 0 && r < l) Decrements remaining buffer size l -= r when advancing pointer p Prevents writing past the end of the tooltip_msg buffer This should resolve the GUI crash when hovering over the graph.
| if(name) { | ||
| p += snprintf(p, l, "name: %s\n", name); | ||
| int r = snprintf(p, l, "name: %s\n", name); | ||
| if(r > 0 && r < l) { |
There was a problem hiding this comment.
What values meet this criteria?
There was a problem hiding this comment.
The condition guards against two distinct snprintf failure/edge cases:
-
r > 0—snprintfreturns a negative value on encoding errors, and0if nothing was written. Advancingpor decrementinglin those cases would be wrong. -
r < l—snprintfreturns the number of characters it would have written if the buffer were large enough, regardless of truncation. So ifr >= l, the output was truncated: onlyl-1bytes were actually written, butris larger thanl. Advancingpbyrwould jump past the end of the buffer.
The combined check r > 0 && r < l means: "snprintf succeeded and the output fit entirely in the remaining buffer" — only then is it safe to advance p by exactly r and subtract r from l.
One subtle implication: if truncation occurs (i.e., r >= l), the pointer p and size l are not updated, so subsequent writes still start at the same (now null-terminated) position. This effectively silently drops the truncated content rather than writing garbage or crashing.
There was a problem hiding this comment.
But an int can’t be both >0 and <1
There was a problem hiding this comment.
r < l
It is not a number, its the letter L!
And don't complain about this, it was not me who introduced the l variable 😄
There was a problem hiding this comment.
No problem!
It is better to be to too cautious than introducing new bugs.
|
>>>> "CaCO3" == CaCO3 ***@***.***> writes:
@caco3 commented on this pull request.
--------------------------------------------------------------------------------------------------
In src/libduc-graph/graph.c:
> @@ -208,9 +208,13 @@ static void gen_tooltip(duc_graph *g, struct duc_size *size, const char *name, d
char *p = g->tooltip_msg;
int l = sizeof(g->tooltip_msg);
if(name) {
- p += snprintf(p, l, "name: %s\n", name);
+ int r = snprintf(p, l, "name: %s\n", name);
+ if(r > 0 && r < l) {
r < l
It is not a number, its the letter L!
And don't complain about this, it was not me who introduced the l variable 😄
So I'd re-work this to use 'len' instead of 'l' in this patch. I'll
look this over more tomorrow. Still stuffed from Sunday Easter dinner.
|
The gen_tooltip function now properly tracks the remaining buffer space:
if(r > 0 && r < l)l -= rwhen advancing pointerpThis should resolve the GUI crash when hovering over the graph.