[PATCH v2 0/1] MR1024: xmllite/writer: Correctly partially duplicate strings
Currently, the code copies one extra character than requested and does not terminate the string. Signed-off-by: David Kahurani <k.kahurani(a)gmail.com> -- v2: xmllite/writer: Correctly partially duplicate strings https://gitlab.winehq.org/wine/wine/-/merge_requests/1024
From: David Kahurani <k.kahurani(a)gmail.com> Signed-off-by: David Kahurani <k.kahurani(a)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; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1024
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10426
Would that mean it doesn't rely on WriteRaw also? Edit : I mean, WriteString? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10428
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10429
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10434
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10435
OOh, I did think about this approach but thought it might duplicate code. And, well, I think I understand the issue with duplicating input... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10441
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10543
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10682
Please push with updated commit message - strings are not being "partially duplicated", but duplicated with explicit length. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1024#note_10684
participants (4)
-
David Kahurani -
David Kahurani (＠kahurani)
-
Marvin -
Nikolay Sivov (＠nsivov)