The comments in the XML declaration test suggest that it is supposed to be to checking for failure when no element follows the XML declaration. However, the error being tested for is due to a nul character in the input stream.
Create a separate test for nul-character errors, and fix the XML declaration test accordingly.
Signed-off-by: Jeff Smith whydoubt@gmail.com --- dlls/xmllite/tests/reader.c | 83 +++++++++++++++++++++++++++++-------- 1 file changed, 66 insertions(+), 17 deletions(-)
diff --git a/dlls/xmllite/tests/reader.c b/dlls/xmllite/tests/reader.c index 41adad1598..88b9103e1e 100644 --- a/dlls/xmllite/tests/reader.c +++ b/dlls/xmllite/tests/reader.c @@ -863,7 +863,6 @@ static void test_read_xmldeclaration(void) { {'s','t','a','n','d','a','l','o','n','e',0}, {'y','e','s',0} } }; IXmlReader *reader; - IStream *stream; HRESULT hr; XmlNodeType type; UINT count = 0, len, i; @@ -873,10 +872,7 @@ static void test_read_xmldeclaration(void) hr = CreateXmlReader(&IID_IXmlReader, (LPVOID*)&reader, NULL); ok(hr == S_OK, "Expected S_OK, got %08x\n", hr);
- stream = create_stream_on_data(xmldecl_full, sizeof(xmldecl_full)); - - hr = IXmlReader_SetInput(reader, (IUnknown*)stream); - ok(hr == S_OK, "Expected S_OK, got %08x\n", hr); + set_input_string(reader, xmldecl_full);
hr = IXmlReader_GetAttributeCount(reader, &count); ok(hr == S_OK, "got %08x\n", hr); @@ -968,19 +964,19 @@ static void test_read_xmldeclaration(void)
type = XmlNodeType_XmlDeclaration; hr = IXmlReader_Read(reader, &type); - /* newer versions return syntax error here cause document is incomplete, - it makes more sense than invalid char error */ + ok(hr == S_OK, "got %08x\n", hr); + ok(type == XmlNodeType_Whitespace, "expected XmlNodeType_Whitespace, got %s\n", type_to_str(type)); + + type = XmlNodeType_Whitespace; + hr = IXmlReader_Read(reader, &type); todo_wine { - ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "got 0x%08x\n", hr); + ok(hr == WC_E_ROOTELEMENT, "got %08x\n", hr); ok(type == XmlNodeType_None, "got %d\n", type); + TEST_READER_STATE(reader, XmlReadState_Error); } - IStream_Release(stream);
/* test short variant */ - stream = create_stream_on_data(xmldecl_short, sizeof(xmldecl_short)); - - hr = IXmlReader_SetInput(reader, (IUnknown *)stream); - ok(hr == S_OK, "expected S_OK, got %08x\n", hr); + set_input_string(reader, xmldecl_short);
read_node(reader, XmlNodeType_XmlDeclaration); TEST_READER_POSITION2(reader, 1, 3, ~0u, 21); @@ -1027,14 +1023,66 @@ todo_wine {
type = -1; hr = IXmlReader_Read(reader, &type); -todo_wine - ok(hr == WC_E_SYNTAX || hr == WC_E_XMLCHARACTER /* XP */, "expected WC_E_SYNTAX, got %08x\n", hr); + ok(hr == S_FALSE, "expected S_FALSE, got %08x\n", hr); ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type)); - TEST_READER_POSITION(reader, 1, 41); + + IXmlReader_Release(reader); +} + +static void test_read_nul(void) +{ + static const char xml_empty[] = "<a/>"; + static const char xml_ws[] = "<a/> "; + static const char xml_comment[] = "<a/><!-- comment -->"; + IXmlReader *reader; + XmlNodeType type; + IStream *stream; + HRESULT hr; + + hr = CreateXmlReader(&IID_IXmlReader, (LPVOID*)&reader, NULL); + ok(hr == S_OK, "Expected S_OK, got %08x\n", hr); + + stream = create_stream_on_data(xml_empty, sizeof(xml_empty)); + hr = IXmlReader_SetInput(reader, (IUnknown *)stream); + ok(hr == S_OK, "got %08x\n", hr); + IStream_Release(stream); + + read_node(reader, XmlNodeType_Element); + + type = -1; + hr = IXmlReader_Read(reader, &type); todo_wine - TEST_READER_STATE(reader, XmlReadState_Error); + ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "expected WC_E_SYNTAX, got 0x%08x\n", hr); + ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type));
+ stream = create_stream_on_data(xml_ws, sizeof(xml_ws)); + hr = IXmlReader_SetInput(reader, (IUnknown *)stream); + ok(hr == S_OK, "got %08x\n", hr); IStream_Release(stream); + + read_node(reader, XmlNodeType_Element); + + type = -1; + hr = IXmlReader_Read(reader, &type); +todo_wine { + ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "expected WC_E_SYNTAX, got 0x%08x\n", hr); + ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type)); +} + + stream = create_stream_on_data(xml_comment, sizeof(xml_comment)); + hr = IXmlReader_SetInput(reader, (IUnknown *)stream); + ok(hr == S_OK, "got %08x\n", hr); + IStream_Release(stream); + + read_node(reader, XmlNodeType_Element); + read_node(reader, XmlNodeType_Comment); + + type = -1; + hr = IXmlReader_Read(reader, &type); +todo_wine + ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "expected WC_E_SYNTAX, got 0x%08x\n", hr); + ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type)); + IXmlReader_Release(reader); }
@@ -2672,6 +2720,7 @@ START_TEST(reader) test_read_pending(); test_readvaluechunk(); test_read_xmldeclaration(); + test_read_nul(); test_reader_properties(); test_prefix(); test_namespaceuri();
Signed-off-by: Jeff Smith whydoubt@gmail.com --- dlls/xmllite/reader.c | 11 +++++++++++ dlls/xmllite/tests/reader.c | 4 ---- 2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/dlls/xmllite/reader.c b/dlls/xmllite/reader.c index eddc4d8eec..c8083f63be 100644 --- a/dlls/xmllite/reader.c +++ b/dlls/xmllite/reader.c @@ -1696,6 +1696,14 @@ static HRESULT reader_parse_pi(xmlreader *reader) return S_OK; }
+/* Determine if we are encountering a nul character within the stream */ +static BOOL reader_at_nul(xmlreader *reader) +{ + encoded_buffer *buffer = &reader->input->buffer->utf16; + return (buffer->cur*sizeof(WCHAR) < buffer->written && + *((WCHAR*)buffer->data + buffer->cur) == 0); +} + /* This one is used to parse significant whitespace nodes, like in Misc production */ static HRESULT reader_parse_whitespace(xmlreader *reader) { @@ -1716,6 +1724,7 @@ static HRESULT reader_parse_whitespace(xmlreader *reader) UINT start;
reader_skipspaces(reader); + if (reader_at_nul(reader)) return WC_E_SYNTAX; if (is_reader_pending(reader)) return S_OK;
start = reader->resume[XmlReadResume_Body]; @@ -1766,6 +1775,8 @@ static HRESULT reader_parse_misc(xmlreader *reader) hr = reader_parse_comment(reader); else if (!reader_cmp(reader, piW)) hr = reader_parse_pi(reader); + else if (reader_at_nul(reader)) + return WC_E_SYNTAX; else break;
diff --git a/dlls/xmllite/tests/reader.c b/dlls/xmllite/tests/reader.c index 88b9103e1e..7ad548d081 100644 --- a/dlls/xmllite/tests/reader.c +++ b/dlls/xmllite/tests/reader.c @@ -1051,7 +1051,6 @@ static void test_read_nul(void)
type = -1; hr = IXmlReader_Read(reader, &type); -todo_wine ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "expected WC_E_SYNTAX, got 0x%08x\n", hr); ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type));
@@ -1064,10 +1063,8 @@ todo_wine
type = -1; hr = IXmlReader_Read(reader, &type); -todo_wine { ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "expected WC_E_SYNTAX, got 0x%08x\n", hr); ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type)); -}
stream = create_stream_on_data(xml_comment, sizeof(xml_comment)); hr = IXmlReader_SetInput(reader, (IUnknown *)stream); @@ -1079,7 +1076,6 @@ todo_wine {
type = -1; hr = IXmlReader_Read(reader, &type); -todo_wine ok(hr == WC_E_SYNTAX || broken(hr == WC_E_XMLCHARACTER), "expected WC_E_SYNTAX, got 0x%08x\n", hr); ok(type == XmlNodeType_None, "expected XmlNodeType_None, got %s\n", type_to_str(type));
I don't think it makes sense to test for null explicitly. Any invalid char for given context would be a syntax error.
On Wed, Dec 4, 2019 at 2:04 PM Nikolay Sivov nsivov@codeweavers.com wrote:
I don't think it makes sense to test for null explicitly. Any invalid char for given context would be a syntax error.
Hi Nikolay,
I agree that it should work that way. However, the tests show that it does not. My belief is that the stream is being treated as a string, so when a null character encountered, it is treating as marking the end of the stream, even though it may actually be a part of the stream. My patch makes sure that condition is being covered, at least for the cases that are being tested.
Thanks, Jeff
On 12/5/19 12:30 AM, Jeff Smith wrote:
On Wed, Dec 4, 2019 at 2:04 PM Nikolay Sivov nsivov@codeweavers.com wrote:
I don't think it makes sense to test for null explicitly. Any invalid char for given context would be a syntax error.
Hi Nikolay,
I agree that it should work that way. However, the tests show that it does not. My belief is that the stream is being treated as a string, so when a null character encountered, it is treating as marking the end of the stream, even though it may actually be a part of the stream. My patch makes sure that condition is being covered, at least for the cases that are being tested.
Putting explicit null checks here and there just to fix some tests is not worth it in my opinion. Instead it should be a part of character range checks, as we do now. E.g. while on whitespace, hitting 0 char would mean you're done with whitespace node, and whatever node is allowed or expected next should handle it.
Thanks, Jeff
-
Jeff Smith -
Nikolay Sivov