Currently, the code copies one extra character than requested and does not terminate the string.
Signed-off-by: David Kahurani k.kahurani@gmail.com
-- v2: xmllite/writer: Correctly partially duplicate strings
From: David Kahurani k.kahurani@gmail.com
Signed-off-by: David Kahurani k.kahurani@gmail.com --- dlls/xmllite/writer.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/dlls/xmllite/writer.c b/dlls/xmllite/writer.c index c4f9ed2f440..065716f7e8c 100644 --- a/dlls/xmllite/writer.c +++ b/dlls/xmllite/writer.c @@ -250,7 +250,6 @@ static struct element *pop_element(xmlwriter *writer)
static WCHAR *writer_strndupW(const xmlwriter *writer, const WCHAR *str, int len) { - size_t size; WCHAR *ret;
if (!str) @@ -259,9 +258,12 @@ static WCHAR *writer_strndupW(const xmlwriter *writer, const WCHAR *str, int len if (len == -1) len = lstrlenW(str);
- size = (len + 1) * sizeof(WCHAR); - ret = writer_alloc(writer, size); - if (ret) memcpy(ret, str, size); + ret = writer_alloc(writer, (len + 1) * sizeof(WCHAR)); + if (ret) + { + memcpy(ret, str, len * sizeof(WCHAR)); + ret[len] = 0; + }
return ret; }
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=124923
Your paranoid android.
=== debian11 (build log) ===
Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24822. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24822. Use of uninitialized value $Flaky in addition (+) at /home/testbot/lib/WineTestBot/LogUtils.pm line 720, <$LogFile> line 24822.
If that's for WriteChars(), then you don't need to duplicate anything.
Would that mean it doesn't rely on WriteRaw also?
Edit : I mean, WriteString?
strndup() is used to keep internal structure of namespaces, to check against. I don't see why you need to duplicate anything for raw writing methods.
A method like WriteChars accepts a string and the number of characters to write to the buffer/xml. A method like WriteString accepts a string only and writes the whole string into the buffer/xml. If my speculation above about the possible relation between WriteChars and WriteString is correct, wouldn't mean it you have to make a copy of the string to the exact size(as specified in the number of characters to write) and then pass that to WriteString but *yes*, get rid of it later?
I don't know what WriteChars() does, maybe it should escape as WriteString() does, maybe not. If it does escape, I'd rather change write_escaped_string() to accept explicit length then duplicating input.
OOh, I did think about this approach but thought it might duplicate code. And, well, I think I understand the issue with duplicating input...
On Thu Oct 13 11:52:01 2022 +0000, David Kahurani wrote:
OOh, I did think about this approach but thought it might duplicate code. And, well, I think I understand the issue with duplicating input... Edit : I mean to say : I think I do not understand the issue with duplicating input
This issue is that you allocate/free when there is no need to do that. First it would certainly help to understand what unimplemented calls are supposed to do, when you can call them and so on.
To clarify, I think null terminating is fine here, so patch looks good to me. But other methods you mentioned need more tests first, and most likely no need for calling one from another.
Please push with updated commit message - strings are not being "partially duplicated", but duplicated with explicit length.
-
David Kahurani -
David Kahurani (＠kahurani)
-
Marvin -
Nikolay Sivov (＠nsivov)