•  

Comment Results

Review Name Created Custom Fields Content
CR-2 14 Sep 2012

Move this check up.

CR-2 14 Sep 2012

Put braces around the if statement

CR-2 14 Sep 2012

Why is only one part checked here?

CR-2 14 Sep 2012

All but the last item were checked in L329-334.

CR-2 14 Sep 2012

This check is invalid if Strict = FALSE.
You can pass 192.11010049 in order to get 192.168.0.1

CR-2 14 Sep 2012

The number of parts is counted first, this loop checks all but the last part (and the last part is checked in the switch below).

This isn't particularly obvious. I could move it all into the switch but that's not optimal either. Hmm.

CR-2 14 Sep 2012

Didn't see that part, thanks.
Is > 0xFF allowed for the last part in Strict = TRUE ?

CR-2 14 Sep 2012

No. That should be caught in case 4 below, unless I'm mis-thinking. That Parts == 4 is certain in that case due to the check above

CR-2 14 Sep 2012

If you don't parse the port yet, return 0 in *Port, according to http://msdn.microsoft.com/en-us/library/windows/desktop/aa814459(v=vs.85).aspx :
Port [out]
A pointer where the binary representation of the port number is to be stored. The port number is returned in network byte order. If no port was specified in the string pointed to by the AddressString parameter, then the Port parameter is set to zero.

CR-2 14 Sep 2012

around the 'for'-statement also?

CR-2 14 Sep 2012

I believe you both mean the same. I've fixed it to:

    for (i = 0; i < Parts - 1; i++)
    {
        if (Result[i] > 0xFF)
        {
            Status = STATUS_INVALID_PARAMETER;
            goto Done;
        }
    }
CR-2 14 Sep 2012

Also (from MSDN):
The buffer pointed to by the AddressString parameter may contain the IPv4 address string followed by an optional colon and the string representation of a port number. If a port number string is included in the buffer pointed to by AddressString parameter, the binary representation of the port number is returned in the Port parameter. If the buffer pointed to by AddressString parameter does not contain a port number, a zero is returned in the Port parameter.

I suggest you to use the Terminator parameter to see if it's equal to L":" and call the RtlpStringToULong on the remaining string, probably.

CR-2 14 Sep 2012

ok

CR-2 14 Sep 2012

Yep, just not implemented yet. I was trying to get the non-Ex version into Wine first. Then tests and implementation for -Ex

CR-2 17 Sep 2012

When AJ says "reuse existing code", maybe he means to use/adapt code from RtlUnicodeStringToInteger.

Otherwise, the rest looks good to me.

CR-2 17 Sep 2012

Shouldn't you set pAddress to 0?

CR-2 17 Sep 2012

Hmm indeed. And here I was thinking "my test should catch that". Except it doesn't run on _new

CR-3 14 Sep 2012

Who defines _ALLOW_OMIT_FRAME_POINTER? I don't see it in either GCC or our build files.

CR-3 14 Sep 2012

The clobber string for the flags is "cc".

CR-3 14 Sep 2012

If this was a normal macro, the semicolon would be wrong here. But since you can't/shouldn't do

if (blah) _SEH3_TRY something(); _SEH3_END; else blah();

... not sure, it may even be wrong without it

CR-3 14 Sep 2012

I think we agreed on using

_SEH2_TRY { ... } _SEH2_EXCEPT(...) { } _SEH2_END;

but we have occurences in the code that don't have the terminating ;
Therefore I decided to add it. Don't worry, noone gets hurt

CR-3 14 Sep 2012

Not like it's important... but that \ in the end probably isn't intentional.

CR-3 14 Sep 2012

flags vs cc again.

CR-3 15 Sep 2012

True. This must only be defined, if -fno-omit-frame-pointer is not defined. We define it though, so all is well. Suggestion where to explain that?

CR-3 15 Sep 2012

I found is that "cc" seems to be treated as ecx. I'll investigate that further and add a comment.

CR-3 15 Sep 2012

yep, I'll fix it.

CR-3 18 Sep 2012

Might make sense next to where that compiler option is/would be. Someone changing those in the future (omitting FP could make sense for a release build) is unlikely to look here in any case

CR-3 18 Sep 2012

This tells the compiler to generate CLD instructions as I understand it, not omit them. Is that intentional, seeing how there's the explicit CLD below?

CR-3 18 Sep 2012

Yes. Putting the "cld" instruction explicitly, will make sure, that the code below has it cleared. The attribute tells the compiler not to assume it is cleared when generating string instructions. While the former does not guarantee that there are no string operations before the asm statement (although this is rather unlikely), the attribute does not guarantee that the flag is cleared when calling other functions below, especially since they are not real function calls, but embedded in asm. I just wanted to make sure the flag IS cleared, no matter what.

CR-3 03 Jan 2014

Is everything reviewed here addressed in the active PSEH3 ? Can we close this review now ?

CR-3 08 Jan 2014

no

CR-3 13 Apr 2014

Just for the record, after latest changes, this is not required anymore

CR-5 20 Sep 2012

Empty braces aren't a valid initializer – you want {0} or {NULL} or whatever is appropriate for the type

CR-5 20 Sep 2012

Yes indeed, in C (I've just checked). Otherwise in C++ it's empty braces (search for "Firstly, that's how you do it in C. in C++ this specific little trick translates to MY_STRUCT foo = {}; (since not all data types can be initialized with literal 0 in C++). Secondly, when we are talking about constructor initializer list, the direct analoue would be just to include MY_STRUCT() in the initializer list. – AndreyT Oct 26 '09 at 22:12" in http://stackoverflow.com/questions/112085/is-this-c-structure-initialization-trick-safe ; and see http://www.ex-parrot.com/~chris/random/initialise.html)

I therefore will correct that to

Unknown macro: {0}

since we are in C.
By the way, compilation with empty list works both with GCC and MSVC.

CR-5 20 Sep 2012

This file isn't built in the MSVC build. I'm pretty sure it would complain otherwise

CR-5 20 Sep 2012

'Free' or 'cleanup' allocation.

CR-5 20 Sep 2012

You're right, it's not compiled (I added a #error "You're compiled" inside kbd_cli.c). Mea culpa.
I replaced the initializers by :

Unknown macro: {0, 0, 0}

because KDESCRIPTOR is a struct of three numbers.

CR-5 20 Sep 2012

Insert an empty line

CR-5 20 Sep 2012

Can you explain this change. I know that we have a bit of a mess with the address space stuff, like 3 different possibilities (NULL, MmGetKernelAddressSpace() and some other thing, I can't remember)

CR-5 20 Sep 2012

The corresponding "defect" is that the returned value of MmGetKernelAddressSpace() here isn't used.
If you get a look into MmpPageOutPhysicalAddress() in swapout.c (line 343), you will se that :

  • AddressSpace isn't used but only inside the while (entry != NULL && NT_SUCCESS(Status)) loop.
  • inside this while() loop, you pass a test : if (Process && Address < MmSystemRangeStart)
    o If you success it (and if you don't bail out at the second one), AddressSpace is initialized with &Process->Vm
    o If you fail this test, AddressSpace is (re)initialized with MmGetKernelAddressSpace()
  • Then after this 'if' test, the AddressSpace variable is used (inside the do{}while() loop and just after it, BUT ALWAYS INSIDE the while() loop).
CR-5 20 Sep 2012

and it's for initializing it to something... (or at least we can just leave the thing as it, i.e. PMMSUPPORT AddressSpace; ...)

CR-5 21 Sep 2012

I now remember the reason why I haven't committed this yet:
MSDN says "For this attempt to succeed, the new key must be a direct subkey of the key that is referred to by RootDirectory, and the key that RootDirectory refers to must have been opened for KEY_CREATE_SUB_KEY access."
Is this relevant here?

CR-5 21 Sep 2012

I personally don't see the need to comment ASSERTs, usually. Especially if the comment contains "Sanity check", which is just noise and provides no information whatsoever.

CR-5 21 Sep 2012

The exact quote from MSDN is the following (my comments are added inside []) :

"If the key specified by ObjectAttributes does not exist, the routine attempts to create the key. For this attempt to succeed [i.e. the creation of the key, NOT the opening if it already exists], the new key must be a direct subkey of the key that is referred to by RootDirectory [it IS, because the third parameter of InitializeObjectAttributes is NULL and the name of the key is a fully qualified object name], and the key that RootDirectory refers to must have been opened for KEY_CREATE_SUB_KEY access [this IS the case, because KEY_ALL_ACCESS contains KEY_CREATE_SUB_KEY, see MSDN].
If the specified key already exists, it is opened and its value is not affected in any way."

Therefore, I understand this as the equivalent of :

  • I attempt to open an existing key.
  • If it's ok, then it's ok
  • If it's not (doesn't exist), then try to create it.
CR-5 21 Sep 2012

A fully qualified key name does not mean a "direct subkey" of the rootdirectory.
root = <"\Registry\Machine\Foo"> key = "Bar" -> success
root = NULL, key = "\Registry\Machine\Foo\Bar" -> fail
I.e. you cannot create a path with more than one element.

CR-5 21 Sep 2012

Use ObCloseHandle

CR-5 21 Sep 2012

Why is the temp buffer that obviously only the callee uses even allocated by the caller(s)?
Don't take that as a call to action to change that. I'm sure someone would kill you if you changed that

CR-5 21 Sep 2012

On a kernel handle? Why?
ZwCreateKey -> ZwClose seems most consistent

CR-5 21 Sep 2012

All that Nt/ZwClose does is ObCloseHandle(Handle, ExGetPreviousMode). So in the Zw case it's ObCloseHandle(Handle, KernelMode)
ZwClose is a systemservice, it goes the long way through the systemservice dispatcher, so it's slower.

CR-5 21 Sep 2012

Ok thanks. However for our case, where the key is \\Registry\\Machine\\SYSTEM
MountedDevices, its creation (if it doesn't exist) must work because at this step, the SYSTEM hive is loaded, so the SYSTEM key exists.