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

qrenc: silence gcc -Wstringop-overflow warning #147

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

Conversation

sgn
Copy link

@sgn sgn commented Nov 25, 2019

Increase the size passed to strncat(3) by 1 to silence gcc warning.
The increased value is accounted for the NULL terminator.

This change won't change anything at runtime since strncat(3) will
append that '\0' if size of dst is reached but '\0' is not found.

@fukuchi
Copy link
Owner

fukuchi commented Dec 12, 2019

Thank you @sgn.

According to this stackoverflow, we could use strcat() instead of strncat().
https://stackoverflow.com/questions/53408543/strncat-wformat-overflow-warning-when-using-gcc-8-2-1

Actually, we just use static strings as the src of strncat() and in these case there's no need to calculate the length of the src strings by hand.

Can I have your thoughts?

@sgn
Copy link
Author

sgn commented Dec 12, 2019 via email

@sgn sgn force-pushed the gcc-stringop-overflow-warning branch 2 times, most recently from 7cee9f4 to f5b0a0e Compare December 12, 2019 13:26
@sgn
Copy link
Author

sgn commented Dec 12, 2019

Hi @fukuchi,

I've pushed a series of change to replace strncat with strcpy.

The last commit is intended for not altering options in writeANSI and writeASCII,
if you don't like the last patch, feel free to drop it.

@sgn sgn force-pushed the gcc-stringop-overflow-warning branch from aec9bb6 to 8239441 Compare December 12, 2019 13:58
@Woebbeking
Copy link

Hi,
I just got these warnings and found this request. Why do you use strncat()? To avoid buffer overflows, then keep it but change the size parameter (have a look at https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83404). If you're really sure the buffer is big enough, then just use strcat(). Same with strncpy().
Cheers,
André

@sgn sgn force-pushed the gcc-stringop-overflow-warning branch from 8239441 to 1d73355 Compare January 22, 2021 00:27
@sgn
Copy link
Author

sgn commented Jan 22, 2021

I think the buffer is always large enough. Let's just use strcpy(3).

sgn added 4 commits January 22, 2021 07:42
Clean up all inconsistence code-style for the next refactor:
- whitespace after keyword and before open brace
- no whitespace after ( and before )
- whitespace around binary operator
Modern compiler can optimise those calls, let's not burden ourselves
with this counting.
On a modern compiler, the compiler should be able to figure out both
black_s and white_s are compile time constant and fold this change to
constant. And we can easier argue why we need to allow that much of
memory.

This change also help if we decide to support another kind of weird
terminal color scheme in future.
@sgn sgn force-pushed the gcc-stringop-overflow-warning branch from 1d73355 to 22b6819 Compare January 22, 2021 00:42
sgn added 2 commits January 22, 2021 07:43
strncat(3) needs to traverse through the buffer to append the string
into the end of old string. This will have a big performance penalty
when we have a very large input.

Replace it with strcpy(3) and a pointer to track the end of current
buffer.

Accidently, this change also silence gcc's -Wstringop-overflow warning,
since gcc thinks our issue to strncat(3) to stop right at the NULL
terminator maybe an error.
We're reseting size to 1 in writeANSI and writeASCII, this may be
problematic if someone use qrencode as a library.

Let's just ignore it in ASCII/ANSI mode instead.
@sgn sgn force-pushed the gcc-stringop-overflow-warning branch from 22b6819 to 461d4e8 Compare January 22, 2021 00:44
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.

3 participants