Skip to content
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.

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

Safe sprintf call #164

Closed
klebertarcisio opened this issue Jul 10, 2020 · 6 comments
Closed

Safe sprintf call #164

klebertarcisio opened this issue Jul 10, 2020 · 6 comments

Comments

@klebertarcisio
Copy link
Contributor

klebertarcisio commented Jul 10, 2020

Hi everyone,

I would like to know what do you think about these sprintf calls:

sprintf(
max_aspect, "%d/%d",
size_hints->max_aspect.x,
size_hints->max_aspect.y);

In this case, x and y are integer variables, but max_aspect is a char of just 11 bytes.
See this toy example:
sprintf (mymin_aspect, "% d % d", 111111, 111111);
In this case, we have an overflow. Should the mymin_aspect variable be 24 bytes?

fvwm3/libs/Colorset.c

Lines 111 to 120 in 48dd509

sprintf(csetbuf,
"Colorset "
"%x %lx %lx %lx %lx %lx %lx %lx %lx %lx "
"%x %x %x %x %x %x %x %x %x %x %x",
n, cs->fg, cs->bg, cs->hilite, cs->shadow, cs->fgsh, cs->tint,
cs->icon_tint, cs->pixmap, cs->shape_mask,
cs->fg_alpha_percent, cs->width, cs->height, cs->pixmap_type,
cs->shape_width, cs->shape_height, cs->shape_type,
cs->tint_percent, cs->do_dither_icon, cs->icon_tint_percent,
cs->icon_alpha_percent);

In this case, csetbuf is a char of just 256 bytes, but the operation requires 270 bytes.

sprintf(wm_sx, "WM_S%lu", Scr.screen);

In this case, wm_sx is a char of just 20 bytes, but the operation requires 25 bytes.

Regards

@issue-label-bot
Copy link

Issue Label Bot is not confident enough to auto-label this issue. See dashboard for more details.

@ThomasAdam
Copy link
Member

ThomasAdam commented Jul 10, 2020

Hi @klebertarcisio,

Yes, the compiler is starting to spit these warnings out now, as is the static analysis tool we're using. See:

#107

I'd also suggest snprintf() as well, if we must fix these. Additionally, for transformations in code such as this, I would also rather see our cocinelle scripts updated to support this work.

I'm all for fixing these, however, what I'd rather see are things like this changed as a result of bug-fixes in that area of the code, rather than necessarily going through all of these things piecemeal.

A lot of what you're identifying is code which has been there for many years, and although compilers are now getting smarter, the code itself isn't causing any problems. We will fix these, but I'd much rather see some of the issues blocking a 1.0 release looked at first: https://github.com/fvwmorg/fvwm3/projects/1?card_filter_query=milestone%3A1.0

@ThomasAdam
Copy link
Member

Hi @klebertarcisio,

Now that things have started to settle -- can you revisit this?

@klebertarcisio
Copy link
Contributor Author

Hi @ThomasAdam, yes, I can do it!

@ThomasAdam
Copy link
Member

Hi @klebertarcisio

I hope you're well. Just checking to see if you think you'll be able to look at this issue soon? If not, I'll close this and move this in to the TODO list.

No pressure -- just trying to do some project maintenance.

@klebertarcisio
Copy link
Contributor Author

Hi @ThomasAdam, sorry for my delay. I create a pull request about this issue. Here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants