| Message ID | 1447836791-369-18-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS *** > *** MACROS MAY CONTAIN MALICIOUS CODE *** > *** Open only if you can verify and trust the sender *** > *** Please contact infosec@redhat.com if you have questions or concerns ** Another one. > The method c_name() is supposed to do two different actions: munge > '-' into '_', and add a 'q_' prefix to ticklish names. But it did > these steps out of order, making it possible to submit input that > is not ticklish until after munging, where the output then lacked > the desired prefix. > > The failure is exposed easily if you have a compiler that recognizes > C11 keywords, and try to name a member '_Thread-local', as it would > result in trying to compile the declaration 'uint64_t _Thread_local;' > which is not valid. However, this name violates our conventions > (ultimately, want to enforce that no qapi names start with single > underscore), so the test is slightly weaker by instead testing > 'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where > wchar_t is only a typedef) but fails with a C++ compiler (where it > is a keyword). Do we support compiling it with a C++ compiler? To sidestep the question, I'd say "but would fail". In my private opinion, the one sane way to compile C code with a C++ compiler is wrapping it in extern "C" { ... }. > Fix things by reversing the order of actions within c_name(). > > Signed-off-by: Eric Blake <eblake@redhat.com> Again, just commit message polish, can do on merge.
On 11/18/2015 03:18 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS *** >> *** MACROS MAY CONTAIN MALICIOUS CODE *** >> *** Open only if you can verify and trust the sender *** >> *** Please contact infosec@redhat.com if you have questions or concerns ** > > Another one. > >> The method c_name() is supposed to do two different actions: munge >> '-' into '_', and add a 'q_' prefix to ticklish names. But it did >> these steps out of order, making it possible to submit input that >> is not ticklish until after munging, where the output then lacked >> the desired prefix. >> >> The failure is exposed easily if you have a compiler that recognizes >> C11 keywords, and try to name a member '_Thread-local', as it would >> result in trying to compile the declaration 'uint64_t _Thread_local;' >> which is not valid. However, this name violates our conventions >> (ultimately, want to enforce that no qapi names start with single >> underscore), so the test is slightly weaker by instead testing >> 'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where >> wchar_t is only a typedef) but fails with a C++ compiler (where it >> is a keyword). > > Do we support compiling it with a C++ compiler? To sidestep the > question, I'd say "but would fail". I know we require a C++ compiler for qga on Windows, and qga uses qapi, so I think the problem is real; but as I do not have a working setup for compiling qga for windows, I can only speculate. Changing s/fails/but would fail/ is a nice hedge. > > In my private opinion, the one sane way to compile C code with a C++ > compiler is wrapping it in extern "C" { ... }. True - but I don't think that changes the set of C++ keywords inside the extern block, does it? (The thought of context-sensitive keywords makes me cringe for how one would write a sane parser for that language). > >> Fix things by reversing the order of actions within c_name(). >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > Again, just commit message polish, can do on merge. > Don't know why you got it on some messages and not others; I also got a round of those pollutions on other threads. It looks like the responsible party has cleaned up their false positives in the meantime, so hopefully we aren't hit by more of the annoyance.
Eric Blake <eblake@redhat.com> writes: > On 11/18/2015 03:18 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> *** WARNING: THE ATTACHED DOCUMENT(S) CONTAIN MACROS *** >>> *** MACROS MAY CONTAIN MALICIOUS CODE *** >>> *** Open only if you can verify and trust the sender *** >>> *** Please contact infosec@redhat.com if you have questions or concerns ** >> >> Another one. >> >>> The method c_name() is supposed to do two different actions: munge >>> '-' into '_', and add a 'q_' prefix to ticklish names. But it did >>> these steps out of order, making it possible to submit input that >>> is not ticklish until after munging, where the output then lacked >>> the desired prefix. >>> >>> The failure is exposed easily if you have a compiler that recognizes >>> C11 keywords, and try to name a member '_Thread-local', as it would >>> result in trying to compile the declaration 'uint64_t _Thread_local;' >>> which is not valid. However, this name violates our conventions >>> (ultimately, want to enforce that no qapi names start with single >>> underscore), so the test is slightly weaker by instead testing >>> 'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where >>> wchar_t is only a typedef) but fails with a C++ compiler (where it >>> is a keyword). >> >> Do we support compiling it with a C++ compiler? To sidestep the >> question, I'd say "but would fail". > > I know we require a C++ compiler for qga on Windows, and qga uses qapi, > so I think the problem is real; but as I do not have a working setup for > compiling qga for windows, I can only speculate. Changing s/fails/but > would fail/ is a nice hedge. > >> >> In my private opinion, the one sane way to compile C code with a C++ >> compiler is wrapping it in extern "C" { ... }. > > True - but I don't think that changes the set of C++ keywords inside the > extern block, does it? (The thought of context-sensitive keywords makes > me cringe for how one would write a sane parser for that language). The truly sane way to compile C with a C++ compiler is of course "not": compile it with C and link it. The exception is headers. Stick to declaring extern symbols and the types they need. >>> Fix things by reversing the order of actions within c_name(). >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> Again, just commit message polish, can do on merge. >> > > Don't know why you got it on some messages and not others; I also got a > round of those pollutions on other threads. It looks like the > responsible party has cleaned up their false positives in the meantime, > so hopefully we aren't hit by more of the annoyance. I reported it, and was told it's been fixed.
diff --git a/scripts/qapi.py b/scripts/qapi.py index b068141..4a30bc0 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -1481,10 +1481,11 @@ def c_name(name, protect=True): 'not_eq', 'or', 'or_eq', 'xor', 'xor_eq']) # namespace pollution: polluted_words = set(['unix', 'errno']) + name = name.translate(c_name_trans) if protect and (name in c89_words | c99_words | c11_words | gcc_words | cpp_words | polluted_words): return "q_" + name - return name.translate(c_name_trans) + return name eatspace = '\033EATSPACE.' pointer_suffix = ' *' + eatspace diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index 44638da..4b89527 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -152,12 +152,13 @@ { 'event': 'EVENT_D', 'data': { 'a' : 'EventStructOne', 'b' : 'str', '*c': 'str', '*enum3': 'EnumOne' } } -# test that we correctly compile downstream extensions +# test that we correctly compile downstream extensions, as well as munge +# ticklish names { 'enum': '__org.qemu_x-Enum', 'data': [ '__org.qemu_x-value' ] } { 'struct': '__org.qemu_x-Base', 'data': { '__org.qemu_x-member1': '__org.qemu_x-Enum' } } { 'struct': '__org.qemu_x-Struct', 'base': '__org.qemu_x-Base', - 'data': { '__org.qemu_x-member2': 'str' } } + 'data': { '__org.qemu_x-member2': 'str', '*wchar-t': 'int' } } { 'union': '__org.qemu_x-Union1', 'data': { '__org.qemu_x-branch': 'str' } } { 'struct': '__org.qemu_x-Struct2', 'data': { 'array': ['__org.qemu_x-Union1'] } } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 786024e..0724a9f 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -185,6 +185,7 @@ enum __org.qemu_x-Enum ['__org.qemu_x-value'] object __org.qemu_x-Struct base __org.qemu_x-Base member __org.qemu_x-member2: str optional=False + member wchar-t: int optional=True object __org.qemu_x-Struct2 member array: __org.qemu_x-Union1List optional=False object __org.qemu_x-Union1 diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c index 888fb5f..9f35b80 100644 --- a/tests/test-qmp-commands.c +++ b/tests/test-qmp-commands.c @@ -65,6 +65,10 @@ __org_qemu_x_Union1 *qmp___org_qemu_x_command(__org_qemu_x_EnumList *a, ret->type = ORG_QEMU_X_UNION1_KIND___ORG_QEMU_X_BRANCH; ret->u.__org_qemu_x_branch = strdup("blah1"); + /* Also test that 'wchar-t' was munged to 'q_wchar_t' */ + if (b && b->value && !b->value->has_q_wchar_t) { + b->value->q_wchar_t = 1; + } return ret; }
The method c_name() is supposed to do two different actions: munge '-' into '_', and add a 'q_' prefix to ticklish names. But it did these steps out of order, making it possible to submit input that is not ticklish until after munging, where the output then lacked the desired prefix. The failure is exposed easily if you have a compiler that recognizes C11 keywords, and try to name a member '_Thread-local', as it would result in trying to compile the declaration 'uint64_t _Thread_local;' which is not valid. However, this name violates our conventions (ultimately, want to enforce that no qapi names start with single underscore), so the test is slightly weaker by instead testing 'wchar-t'; the declaration 'uint64_t wchar_t;' is valid in C (where wchar_t is only a typedef) but fails with a C++ compiler (where it is a keyword). Fix things by reversing the order of actions within c_name(). Signed-off-by: Eric Blake <eblake@redhat.com> --- v12: new patch --- scripts/qapi.py | 3 ++- tests/qapi-schema/qapi-schema-test.json | 5 +++-- tests/qapi-schema/qapi-schema-test.out | 1 + tests/test-qmp-commands.c | 4 ++++ 4 files changed, 10 insertions(+), 3 deletions(-)