Small fixes for LCD text printing #155
Conversation
…flow for larger offsets)
This reverts commit 2eed223.
| @@ -118,7 +118,7 @@ int main(void) { | |||
| LCD_disp_str((uint8_t*)buf, len, 0, 64 - 6, FONT6X6); | |||
| printf("\nRunning on an %s", buf); | |||
|
|
|||
| len = snprintf(buf, sizeof(buf), "%s", Version_GetGitVersion()); | |||
There was a problem hiding this comment.
This introduces a potential buyer overflow for future devs, try sizeof(buf) - 1
There was a problem hiding this comment.
Oh wow, that was fast
As far as I have understood the original problem, buf is not the
problem. The problem is that the LCD_disp_str() function also uses fixed
length buffers according to display pixel size and these buffers will
overflow if you try to print texts resulting in more pixels than the
display width. At a 6x6 font the maximum is 21 characters, that's why I
limited to it.
Does that explain it a bit?
There was a problem hiding this comment.
In that case, I would suggest introducing a common define used between the output code and buffer allocation, which makes the relationship explicit
|
You don't need to worry about using sizeof(buf)-1, as the snprintf function handles that for you. Just pass the size of the buffer, and it reserves enough space for the terminating null character according to this reference: http://www.cplusplus.com/reference/cstdio/snprintf/. |
|
@mikeanton that's C++, the C implementation does not guarantee a null terminated string on truncation. https://linux.die.net/man/3/snprintf |
|
@deece The reference you linked to says exactly the same thing: that the number of bytes written includes the terminating null character. Therefore, using sizeof(buf)-1 is not required. |
|
You're right, i just confirmed that a truncated string format will insert a null byte. It seems I've been tainted by _snprintf from Microsoft :) |
|
Seems like @xnk is not responding anymore :( Would love to get this merged. |
|
I’m here, buried in a lot of other stuff at the moment (and has been for quite some time), sorry about that! Need to try this out and we’ll get this merged!
/wj
… On Feb 13, 2019, at 7:28 PM, Moritz Stückler ***@***.***> wrote:
Seems like @xnk <https://github.com/xnk> is not responding anymore :( Would love to get this merged.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#155 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADEoj2XvtIMnzVeyFenjuI52pp_DUV3Tks5vNFlDgaJpZM4aJ1Hn>.
|
|
Thanks @xnk! Appreciate your work! |
This PR includes two smaller fixes around printing of text.
First fix is resolving buffer overflows when compiling from GIT and Version_GetGitVersion() returns a pretty long string, which overflows the LCD_disp_str buffer (and causes a crash and reset).
The second fix makes some more room in the setup screen so that higher offsets with one digit more can be displayed - previously it crashed for me too for offsets not fitting the screen anymore.