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

Fix buffer overflow in LCD_disp_str #245

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mcapdeville
Copy link

This buffer overflow occur when right tc offet is >= 10.00 or <=-10.00 in this case, there is 22 chars to be displayed on the line. This also occur with long version string as v0.5.2-4-dirty-gxxxxxx. This cause reboot of the firmware.

note from snprintf(3) :
snprintf return the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available.

@borland1
Copy link

borland1 commented Nov 9, 2023

I think this is what you're describing as frame buffer overflow for the two OFFSET setup values.

T962SetupMenu1

Your proposed code changes do eliminate the buffer overflow, but don't address the source of the problem.

The reason for the OFFSET character overflow (when offset values are too positive or too negative) is that the Setup menu display format in file Setup.c, has formating errors. Here's the format code with two lines that need changes.

static setupMenuStruct setupmenu[] = {
	{"Min fan speed    %4.0f", REFLOW_MIN_FAN_SPEED, 0, 254, 0, 1.0f},
	{"Cycle done beep %4.1fs", REFLOW_BEEP_DONE_LEN, 0, 254, 0, 0.1f},
	{"Left TC gain     %1.2f", TC_LEFT_GAIN, 10, 190, 0, 0.01f},
	{"Left TC offset  %+1.2f", TC_LEFT_OFFSET, 0, 200, -100, 0.25f},       //   <---
	{"Right TC gain    %1.2f", TC_RIGHT_GAIN, 10, 190, 0, 0.01f},
	{"Right TC offset %+1.2f", TC_RIGHT_OFFSET, 0, 200, -100, 0.25f},  //   <---
};

You can see that the OFFSET input values for both left and right TCs are in two decimal places ( 0.01 degrees C ) and in increments of 0.25C. Displaying the OFFSET values with 2 decimal places is too precise.

T962SetupMenu2

If you substitute these lines in the Strut above,

	{"Left TC offset  %+1.1f", TC_LEFT_OFFSET, 0, 250, -125, 0.20f},   //  <-------

	{"Right TC offset %+1.1f", TC_RIGHT_OFFSET, 0, 250, -125, 0.20f},  //  <-------

the display will look like this..

T962SetupMenu3

while still having the same input value range, but with 0.2C increments.

T962SetupMenu4

The probleme occur when right tc offet is >= 10.00 or <=-10.00
or when version string is more than 21 char as v0.5.2-4-dirty-gxxxxxx.
This cause reboot of the firmware.

The buffer overflow is triggered when trying to print the null char at
the end of the string.

note from snprintf(3) :
snprintf return the return value is the number of characters (excluding the terminating null byte) which would have  been
written  to the final string if enough space had been available.
Tc Offset can be adjusted to +-62.5°C.
To fit in unsigned char, precision is reduce to 0.5°C
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

Successfully merging this pull request may close these issues.

2 participants