| Message ID | 20181218110333.22558-1-philmd@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix strncpy() warnings for GCC8 new -Wstringop-truncation | expand |
On Tue, Dec 18, 2018 at 12:03:30PM +0100, Philippe Mathieu-Daudé wrote: > GCC 8 new warning prevents builds to success since quite some time. > First report on the mailing list is in July 2018: > https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03723.html > > Various intents has been sent to fix this: > - Incorrectly using g_strlcpy() > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03705.html > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03706.html > - Using assert() and strpadcpy() > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03938.html > - Use #pragma GCC diagnostic ignored "-Wstringop-truncation" > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > - adding an inline wrapper with said pragma in there > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > - -Wno-stringop-truncation is the makefile > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > This series replace the strncpy() calls by strpadcpy() which seemed > to me the saniest option. > > Regards, > > Phil. Do you happen to know why does it build fine with Gcc 8.2.1? Reading the GCC manual it seems that there is a "nostring" attribute that means "might not be 0 terminated". I think we should switch to that which fixes the warning but also warns if someone tries to misuse these as C-strings. Seems to be a better option, does it not? > Marc-André Lureau (1): > hw/acpi: Replace strncpy() by strpadcpy(pad='\0') > > Philippe Mathieu-Daudé (2): > block/sheepdog: Replace strncpy() by strpadcpy(pad='\0') > migration: Replace strncpy() by strpadcpy(pad='\0') > > block/sheepdog.c | 6 +++--- > hw/acpi/aml-build.c | 6 ++++-- > hw/acpi/core.c | 13 +++++++------ > migration/global_state.c | 4 ++-- > 4 files changed, 16 insertions(+), 13 deletions(-) > > -- > 2.17.2
On Tue, Dec 18, 2018 at 09:31:00AM -0500, Michael S. Tsirkin wrote: > On Tue, Dec 18, 2018 at 12:03:30PM +0100, Philippe Mathieu-Daudé wrote: > > GCC 8 new warning prevents builds to success since quite some time. > > First report on the mailing list is in July 2018: > > https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03723.html > > > > Various intents has been sent to fix this: > > - Incorrectly using g_strlcpy() > > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03705.html > > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03706.html > > - Using assert() and strpadcpy() > > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03938.html > > - Use #pragma GCC diagnostic ignored "-Wstringop-truncation" > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > - adding an inline wrapper with said pragma in there > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > - -Wno-stringop-truncation is the makefile > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > > > This series replace the strncpy() calls by strpadcpy() which seemed > > to me the saniest option. > > > > Regards, > > > > Phil. > > Do you happen to know why does it build fine with > Gcc 8.2.1? > > Reading the GCC manual it seems that > there is a "nostring" attribute that means typo - its "nonstring" > "might not be 0 terminated". > I think we should switch to that which fixes the warning > but also warns if someone tries to misuse these > as C-strings. > > Seems to be a better option, does it not? Yes, it does look best as gcc manual explicitly suggests "nonstring" as the way to stop this strncpy warning. Regards, Daniel
On Tue, Dec 18, 2018 at 09:31:00AM -0500, Michael S. Tsirkin wrote: > On Tue, Dec 18, 2018 at 12:03:30PM +0100, Philippe Mathieu-Daudé wrote: > > GCC 8 new warning prevents builds to success since quite some time. > > First report on the mailing list is in July 2018: > > https://lists.gnu.org/archive/html/qemu-devel/2018-07/msg03723.html > > > > Various intents has been sent to fix this: > > - Incorrectly using g_strlcpy() > > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03705.html > > https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03706.html > > - Using assert() and strpadcpy() > > https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03938.html > > - Use #pragma GCC diagnostic ignored "-Wstringop-truncation" > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > - adding an inline wrapper with said pragma in there > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > - -Wno-stringop-truncation is the makefile > > https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg04261.html > > > > This series replace the strncpy() calls by strpadcpy() which seemed > > to me the saniest option. > > > > Regards, > > > > Phil. > > Do you happen to know why does it build fine with > Gcc 8.2.1? > > Reading the GCC manual it seems that > there is a "nostring" attribute Sorry that should be "nonstring". > that means > "might not be 0 terminated". > I think we should switch to that which fixes the warning > but also warns if someone tries to misuse these > as C-strings. > > Seems to be a better option, does it not? > Also maybe we can make strpadcpy check for the nonstring attribute too somehow? Or did gcc just hardcode the strncpy name? > > Marc-André Lureau (1): > > hw/acpi: Replace strncpy() by strpadcpy(pad='\0') > > > > Philippe Mathieu-Daudé (2): > > block/sheepdog: Replace strncpy() by strpadcpy(pad='\0') > > migration: Replace strncpy() by strpadcpy(pad='\0') > > > > block/sheepdog.c | 6 +++--- > > hw/acpi/aml-build.c | 6 ++++-- > > hw/acpi/core.c | 13 +++++++------ > > migration/global_state.c | 4 ++-- > > 4 files changed, 16 insertions(+), 13 deletions(-) > > > > -- > > 2.17.2
On 18/12/18 15:31, Michael S. Tsirkin wrote: > Do you happen to know why does it build fine with > Gcc 8.2.1? > > Reading the GCC manual it seems that > there is a "nostring" attribute that means > "might not be 0 terminated". > I think we should switch to that which fixes the warning > but also warns if someone tries to misuse these > as C-strings. > > Seems to be a better option, does it not? > > Using strpadcpy is clever and self-documenting, though. We have it already, so why not use it. Paolo
On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote: > On 18/12/18 15:31, Michael S. Tsirkin wrote: > > Do you happen to know why does it build fine with > > Gcc 8.2.1? > > > > Reading the GCC manual it seems that > > there is a "nostring" attribute that means > > "might not be 0 terminated". > > I think we should switch to that which fixes the warning > > but also warns if someone tries to misuse these > > as C-strings. > > > > Seems to be a better option, does it not? > > > > > > Using strpadcpy is clever and self-documenting, though. We have it > already, so why not use it. > > Paolo The advantage of nonstring is that it will catch attempts to use these fields with functions that expect a 0 terminated string. strpadcpy will instead just silence the warning.
On 18/12/18 15:54, Michael S. Tsirkin wrote: > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote: >> On 18/12/18 15:31, Michael S. Tsirkin wrote: >>> Do you happen to know why does it build fine with >>> Gcc 8.2.1? >>> >>> Reading the GCC manual it seems that >>> there is a "nostring" attribute that means >>> "might not be 0 terminated". >>> I think we should switch to that which fixes the warning >>> but also warns if someone tries to misuse these >>> as C-strings. >>> >>> Seems to be a better option, does it not? >>> >>> >> >> Using strpadcpy is clever and self-documenting, though. We have it >> already, so why not use it. >> >> Paolo > > The advantage of nonstring is that it will catch attempts to > use these fields with functions that expect a 0 terminated string. > > strpadcpy will instead just silence the warning. Ah, I see. We could also do both, that's a matter of taste. Paolo
On 12/18/18 3:54 PM, Michael S. Tsirkin wrote: > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote: >> On 18/12/18 15:31, Michael S. Tsirkin wrote: >>> Do you happen to know why does it build fine with >>> Gcc 8.2.1? >>> >>> Reading the GCC manual it seems that >>> there is a "nostring" attribute that means >>> "might not be 0 terminated". >>> I think we should switch to that which fixes the warning >>> but also warns if someone tries to misuse these >>> as C-strings. >>> >>> Seems to be a better option, does it not? >>> >>> >> >> Using strpadcpy is clever and self-documenting, though. We have it >> already, so why not use it. >> >> Paolo > > The advantage of nonstring is that it will catch attempts to > use these fields with functions that expect a 0 terminated string. > > strpadcpy will instead just silence the warning. migration/global_state.c:109:15: error: 'strlen' argument 1 declared attribute 'nonstring' [-Werror=stringop-overflow=] s->size = strlen((char *)s->runstate) + 1; ^~~~~~~~~~~~~~~~~~~~~~~~~~~ GCC won... It is true this strlen() is buggy, indeed s->runstate might be not NUL-terminated.
On Tue, Dec 18, 2018 at 05:38:17PM +0100, Paolo Bonzini wrote: > On 18/12/18 15:54, Michael S. Tsirkin wrote: > > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote: > >> On 18/12/18 15:31, Michael S. Tsirkin wrote: > >>> Do you happen to know why does it build fine with > >>> Gcc 8.2.1? > >>> > >>> Reading the GCC manual it seems that > >>> there is a "nostring" attribute that means > >>> "might not be 0 terminated". > >>> I think we should switch to that which fixes the warning > >>> but also warns if someone tries to misuse these > >>> as C-strings. > >>> > >>> Seems to be a better option, does it not? > >>> > >>> > >> > >> Using strpadcpy is clever and self-documenting, though. We have it > >> already, so why not use it. > >> > >> Paolo > > > > The advantage of nonstring is that it will catch attempts to > > use these fields with functions that expect a 0 terminated string. > > > > strpadcpy will instead just silence the warning. > > Ah, I see. We could also do both, that's a matter of taste. > > Paolo Do you happen to know how to make gcc check the buffer size for strpadcpy? Is the name strncpy just hard-coded?
On Tue, Dec 18, 2018 at 05:55:27PM +0100, Philippe Mathieu-Daudé wrote: > On 12/18/18 3:54 PM, Michael S. Tsirkin wrote: > > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote: > >> On 18/12/18 15:31, Michael S. Tsirkin wrote: > >>> Do you happen to know why does it build fine with > >>> Gcc 8.2.1? > >>> > >>> Reading the GCC manual it seems that > >>> there is a "nostring" attribute that means > >>> "might not be 0 terminated". > >>> I think we should switch to that which fixes the warning > >>> but also warns if someone tries to misuse these > >>> as C-strings. > >>> > >>> Seems to be a better option, does it not? > >>> > >>> > >> > >> Using strpadcpy is clever and self-documenting, though. We have it > >> already, so why not use it. > >> > >> Paolo > > > > The advantage of nonstring is that it will catch attempts to > > use these fields with functions that expect a 0 terminated string. > > > > strpadcpy will instead just silence the warning. > > migration/global_state.c:109:15: error: 'strlen' argument 1 declared > attribute 'nonstring' [-Werror=stringop-overflow=] > s->size = strlen((char *)s->runstate) + 1; > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > GCC won... It is true this strlen() is buggy, indeed s->runstate might > be not NUL-terminated. Ooh nice. I smell some CVE fixes coming from this effort.
On 18/12/18 17:55, Philippe Mathieu-Daudé wrote: >> strpadcpy will instead just silence the warning. > migration/global_state.c:109:15: error: 'strlen' argument 1 declared > attribute 'nonstring' [-Werror=stringop-overflow=] > s->size = strlen((char *)s->runstate) + 1; > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > GCC won... It is true this strlen() is buggy, indeed s->runstate might > be not NUL-terminated. No, runstate is declared as an array of 100 bytes, which are more than enough. It's ugly code but not buggy. Paolo
On Tue, Dec 18, 2018 at 06:12:05PM +0100, Paolo Bonzini wrote: > On 18/12/18 17:55, Philippe Mathieu-Daudé wrote: > >> strpadcpy will instead just silence the warning. > > migration/global_state.c:109:15: error: 'strlen' argument 1 declared > > attribute 'nonstring' [-Werror=stringop-overflow=] > > s->size = strlen((char *)s->runstate) + 1; > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > GCC won... It is true this strlen() is buggy, indeed s->runstate might > > be not NUL-terminated. > > No, runstate is declared as an array of 100 bytes, which are more than > enough. It's ugly code but not buggy. > > Paolo Yes ... but it is loaded using VMSTATE_BUFFER(runstate, GlobalState), and parsed using qapi_enum_parse which does not get the buffer length. So unless we are lucky there's a buffer overrun on a remote/file input here. Seems buggy to me - what am I missing?
On Tue, Dec 18, 2018 at 12:03:34PM -0500, Michael S. Tsirkin wrote: > On Tue, Dec 18, 2018 at 05:38:17PM +0100, Paolo Bonzini wrote: > > On 18/12/18 15:54, Michael S. Tsirkin wrote: > > > On Tue, Dec 18, 2018 at 03:45:08PM +0100, Paolo Bonzini wrote: > > >> On 18/12/18 15:31, Michael S. Tsirkin wrote: > > >>> Do you happen to know why does it build fine with > > >>> Gcc 8.2.1? > > >>> > > >>> Reading the GCC manual it seems that > > >>> there is a "nostring" attribute that means > > >>> "might not be 0 terminated". > > >>> I think we should switch to that which fixes the warning > > >>> but also warns if someone tries to misuse these > > >>> as C-strings. > > >>> > > >>> Seems to be a better option, does it not? > > >>> > > >>> > > >> > > >> Using strpadcpy is clever and self-documenting, though. We have it > > >> already, so why not use it. > > >> > > >> Paolo > > > > > > The advantage of nonstring is that it will catch attempts to > > > use these fields with functions that expect a 0 terminated string. > > > > > > strpadcpy will instead just silence the warning. > > > > Ah, I see. We could also do both, that's a matter of taste. > > > > Paolo > > Do you happen to know how to make gcc check the buffer size > for strpadcpy? Is the name strncpy just hard-coded? GCC provides strncpy as a builtin function and its warning only checks its builtin. Regards, Daniel
On 18/12/18 18:17, Michael S. Tsirkin wrote: > On Tue, Dec 18, 2018 at 06:12:05PM +0100, Paolo Bonzini wrote: >> On 18/12/18 17:55, Philippe Mathieu-Daudé wrote: >>>> strpadcpy will instead just silence the warning. >>> migration/global_state.c:109:15: error: 'strlen' argument 1 declared >>> attribute 'nonstring' [-Werror=stringop-overflow=] >>> s->size = strlen((char *)s->runstate) + 1; >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> GCC won... It is true this strlen() is buggy, indeed s->runstate might >>> be not NUL-terminated. >> >> No, runstate is declared as an array of 100 bytes, which are more than >> enough. It's ugly code but not buggy. >> >> Paolo > > Yes ... but it is loaded using > VMSTATE_BUFFER(runstate, GlobalState), > and parsed using qapi_enum_parse which does not get > the buffer length. > > So unless we are lucky there's a buffer overrun > on a remote/file input here. > > Seems buggy to me - what am I missing? Yup. I think we're lucky twice though. First, the state field stops the runaway qapi_enum_parse. Second, in any case worst case it's a segv on migration. This is a bug but not a CVE. Paolo