Skip to content

Crash on tooltip hovering: snprintf buffer overflow fix #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
caco3 wants to merge 1 commit into
base: v1.5.0-rc2
Choose a base branch
from
Open

Crash on tooltip hovering: snprintf buffer overflow fix #350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/libduc-graph/graph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Copy Markdown
Contributor

@jmtd jmtd Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What values meet this criteria?

Copy link
Copy Markdown
Author

@caco3 caco3 Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition guards against two distinct snprintf failure/edge cases:

  • r > 0 — snprintf returns a negative value on encoding errors, and 0 if nothing was written. Advancing p or decrementing l in those cases would be wrong.

  • r < l — snprintf returns the number of characters it would have written if the buffer were large enough, regardless of truncation. So if r >= l, the output was truncated: only l-1 bytes were actually written, but r is larger than l. Advancing p by r would 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.

Copy link
Copy Markdown
Contributor

@jmtd jmtd Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But an int can’t be both >0 and <1

Copy link
Copy Markdown
Author

@caco3 caco3 Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Copy Markdown
Contributor

@jmtd jmtd Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it is! Sorry for the noise

Copy link
Copy Markdown
Author

@caco3 caco3 Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem!
It is better to be to too cautious than introducing new bugs.

p += r;
l -= r;
}
}
p += snprintf(p, l,
int r = snprintf(p, l,
"type: %s\n"
"actual size: %s\n"
"apparent size: %s\n"
Expand Down