victor martinez calvo

Yup. My editing crossed with this slow CR motion https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/biggrin.gif Yup, I think that an ASSERT to make it more readable could be...

Yup. My editing crossed with this slow CR motion Yup, I think that an ASSERT to make it more readable could be a nice idea but just that. Thanks for your cool work

After this line I'd suggest placing an ASSERT to ensure PortImplemented != 0, otherwise portCount will lead to a division by zero afterwards. If PortImplemented could be zero, and no action is expe...

After this line I'd suggest placing an ASSERT to ensure PortImplemented != 0, otherwise portCount will lead to a division by zero afterwards. If PortImplemented could be zero, and no action is expected in this function, then it should be gracefully handled. EDITED : Oh! I see, the caller prevents the issue since it will return an error in such case. Also I saw hermes comment which is basically the same, imho and just for readibility I'd add the ASSERT so anyone messing later with this function will know that PortImplemented is expected to be > 0 at this point.

Regarding accelerator.c, I made a fix for the TranslateAcceleratorA that nowadays shouldn't be working at all. Link: https://jira.reactos.org/secure/attachment/24705/accel.c.patch

Regarding accelerator.c, I made a fix for the TranslateAcceleratorA that nowadays shouldn't be working at all.
Link: https://jira.reactos.org/secure/attachment/24705/accel.c.patch

Renamed to TestRtlFindCharInUnicodeString..and moved downwards to fit its place

Renamed to TestRtlFindCharInUnicodeString..and moved downwards to fit its place

How?

How?

Hmm...How?I mean, Empty string is not a NULL string, neither a string with content. Maybe ros works correctly with "a\0" but not with just "\0" strings?

Hmm...How?I mean, Empty string is not a NULL string, neither a string with content. Maybe ros works correctly with "a\0" but not with just "\0" strings?

If should be then:ok_eq_long(StringZeroed.Length, sizeof(StringToUpcase2)) no?

If should be then:ok_eq_long(StringZeroed.Length, sizeof(StringToUpcase2)) no?

Right, I "ate" half of the function check...

Right, I "ate" half of the function check...

Yup, it was a copy-pasta of the same structure which didn't give an error because i wrote StringUpcased inside... Removed now https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emotic...

Yup, it was a copy-pasta of the same structure which didn't give an error because i wrote StringUpcased inside...
Removed now

Oh!I didnt know that. So after initializing, the UNICODE_STRING points to the Buffer passed (not to a copy of the buffer, etc).Cool https://code.reactos.org/static/olpro3/2static/images/wiki/icons/...

Oh!I didnt know that. So after initializing, the UNICODE_STRING points to the Buffer passed (not to a copy of the buffer, etc).Cool Thanks.

This is really a bug, the check was really mean to be: ok_eq_wstr(StringToUpcase.Buffer, L"");

This is really a bug, the check was really mean to be: ok_eq_wstr(StringToUpcase.Buffer, L"");

Created two testcases where the Buffer is shorter or longer than "needed" just to show that the API ignores the buffer really, since the API is called with TRUE (Allocate buffer). I will add a POIN...

Created two testcases where the Buffer is shorter or longer than "needed" just to show that the API ignores the buffer really, since the API is called with TRUE (Allocate buffer). I will add a POINTER check too since the StringUpcased prior to the API call should be totally different to StringUpcased after the call (pointing to a different memory address)because the forced allocation.

Sure.Let's focus in those 2 then https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

Sure.Let's focus in those 2 then

What about this Free? The API failed...should we call the RtlFreeUnicodeString? https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

What about this Free? The API failed...should we call the RtlFreeUnicodeString?

Removed https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

Removed

Understood!https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif. Checking just the ANSI_STRING.buffer is not enough, because i can have a "hello" but Length modified...

Understood!.
Checking just the ANSI_STRING.buffer is not enough, because i can have a "hello" but Length modified to 3. So I must check all the variables in the UNICODE_STRING struct, which means: .MaximumLenght value, .Length value, and since Buffer is a pointer: .Buffer content but also .Buffer pointer(address). Because iiuc, we can have an odd case where .MaximumLength and .Length are not modified, and even the Buffer content could be the same but really not using the same address in the memory. So from the content pov the original UNICODE_STRING was unmodified but we're ending with two different addresses of memory having the same content and leaking the original one.

Odd, this ones are misbehaving. Sometimes their value is 65530 other times 65532. They are coherent, aka, running the same kmtest_.exe they always show the same result,ie, 65530 but when adding new...

Odd, this ones are misbehaving. Sometimes their value is 65530 other times 65532. They are coherent, aka, running the same kmtest_.exe they always show the same result,ie, 65530 but when adding new tests, and recompiling, it can fail because it now expects 65532. That looks as a memory issue?overflowing?or something? Windows 2003.

Yeah,Good ones. Added too.

Yeah,Good ones. Added too.

Oh, right, RtlInitUnicodeString will truncate the Max[] and MaxRef[] when initializing the UNICODE_STRING StringToUpcase and StringUpcasedRef, so RtlUpcaseUnicodeString won't deal with the issue I ...

Oh, right, RtlInitUnicodeString will truncate the Max[] and MaxRef[] when initializing the UNICODE_STRING StringToUpcase and StringUpcasedRef, so RtlUpcaseUnicodeString won't deal with the issue I was pretending to test.
Maybe I could use "manual" initializing? aka:
StringToUpcase.Buffer = Max;
StringToUpcase.Length = (UNICODE_STRING_MAX_CHARS + 3) * sizeof(WCHAR);
StringToUpcase.MaximumLength = (UNICODE_STRING_MAX_CHARS +4) * sizeof(WCHAR);
and
RtlUpcaseUnicodeString(&StringUpcased, &StringToUpcase); ?

PS: 3 and 4 are just magic numbers to force the "overflow". Can I use magic numbers in this case?

Well my test was about: The RtlAnsiStringToUnicodeSize recives a pointer to a "hello" ANSI string. ReactOS API calculates correctly that the Length is X, but the API is bugged and in the way it mod...

Well my test was about:
The RtlAnsiStringToUnicodeSize recives a pointer to a "hello" ANSI string. ReactOS API calculates correctly that the Length is X, but the API is bugged and in the way it modifies the "hello" to "hel" string thanks to the pointer provided to the string.
So what I do?
I check that not just the returning value X is calculated correctly but that the Ansi.Buffer after the call still is storing the value used to initialize the ANSI string(called "string" here)
.
And I do it by calling ok_eq_str(Ansi.buffer, string) after the API is done.

(Btw, feel free to correct me in my asumptions, )

This problem won't happen at those APIS with IN and OUT IN/OUT defined at the prototype, since the compiler will detect that the logic is trying to "rewrite" a just input param, as the ANSI in this case, but if the API doesnt have that "decoration?", then we can't be sure that the API hasn't messed the input param. So basically I am not just checking if the return value is correct but if the Input params aren't modified through its pointers provided to the API.
(Maybe too crazy?or even nonsense?)

(Sorry about the length, but lack of English and lack of proper terminology/knowledge is...hehe)

Then I need to add a Comment to prevent misleading...jkjk https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/wink.gif

Then I need to add a Comment to prevent misleading...jkjk

Yup, Well, the idea behind that check is to test if when the API recives a NULL, the content of the StringUpcased suffers any kind of changes. For this purpose I initialized first the StringUpcased...

Yup, Well, the idea behind that check is to test if when the API recives a NULL, the content of the StringUpcased suffers any kind of changes.
For this purpose I initialized first the StringUpcased and after the API call I check if the StringUpcased value is still the same.
For example I was expecting (look the Typo "zero" in the ok) that the StringUpcased.MaximumLength would be set to zero,and thanks to the check I discovered that the value is preserved.
44 is the StringUpcased.MaximumLength before calling the API, the Lenght of L"Ilovetoeaddeadbeef!!!".
Should I create a variable to store this value before calling the API and check against the StringUpcased.MaximumLength then?
Something like:
Prev = StringUpcased.MaximumLength;
RtlUpcaseUnicodeString(blablaba...)
ok(StringUpcased.MaximumLength == Prev) ?

Right.Removing. Also one doubt: Is MaxRef being correctly filled?Now I have doubts. Seems I am filling MaxRef from [0] to UNICODE_STRING_MAX_CHARS-3 with L"R", and then i am adding a L'\0' to UNICO...

Right.Removing.
Also one doubt: Is MaxRef being correctly filled?Now I have doubts. Seems I am filling MaxRef from [0] to UNICODE_STRING_MAX_CHARS-3 with L"R", and then i am adding a L'\0' to UNICODE_STRING_MAX_CHARS-1. What is happening with UNICODE_STRING_MAX_CHARS-2?Uninitialized?
Also, the idea of the MaxRef is to recreate a String which size is the max we can store in a Unicode_string.buffer. Shouldn't we have to use i < UNICODE_STRING_MAX_CHARS -1 and then ending L'\0' to MaxRef[UNICODE_STRING_MAX_CHARS]? Noobs doubts.

Yeah. Much more Pro https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

Yeah. Much more Pro

Oh!Then this is a misconception from my side. What I was trying to check is that the value of the string stored in Ansi.Buffer didn't change. I.e, if the API was called with Ansi.Buffer = L"hello" ...

Oh!Then this is a misconception from my side. What I was trying to check is that the value of the string stored in Ansi.Buffer didn't change. I.e, if the API was called with Ansi.Buffer = L"hello" as param, the Ansi.Buffer stays as L"hello" after the API has finished using that param. Why do I have to check the "pointer"?

Perfect. UCHAR_MAX ftw. Well, I am not looping from 0 to 255 since the behavior from "1 to 255" is different to the behavior of "0". When using "0" the length is "2" while using 1 to 255 the length...

Perfect. UCHAR_MAX ftw.
Well, I am not looping from 0 to 255 since the behavior from "1 to 255" is different to the behavior of "0". When using "0" the length is "2" while using 1 to 255 the length is "4".

Noted and Fixed.

Noted and Fixed.

Yeah, right, I didnt yet update the kmtests. Thanks for the fix! Btw, RtlEqualString and RtlEraseUnicodeString are also needed. Does the commit fix them too?Just in case https://code.reactos.org/st...

Yeah, right, I didnt yet update the kmtests. Thanks for the fix!
Btw, RtlEqualString and RtlEraseUnicodeString are also needed. Does the commit fix them too?Just in case

  • More
  • CR-28
  • finished reviewing
  • More
  • CR-29
  • started review
0.3.15: cmd: ECHO command should write CRLF
0.3.15: cmd: ECHO command should write CRLF