When a KCB (key stuff) is allocated, the key name associated with it receives an NCB (name stuff). In case this name is already used, a cache exists, and an existing NCB is grabbed, and its reference count is increased. When the KCB goes away, its NCB loses a reference. When all references are gone, the NCB is destroyed. Simple enough.
It turns out that what was currently happening is that an NCB would get dereferenced to 0, deleted, but still remained attached to a valid KCB (shouldn't happen). When that KCB went away, the NCB's reference count was dropped to... -1, and then -2, -3, -4, etc. Remember this is a FREED NCB. In other words, freed pool, that might now belong to someone else, was getting "-1" operations on it. So any value stored in that freed pool would get decremented by one. In ARM3 paged pool, because the allocator keeps a linked list, what would happen is that the FLINK pointer would be 0xE0F01234 instead of 0xE1A01234. What happened is that "0xE1A0" was treated as the reference count of the freed NCB, and it kept getting dereferenced down to 0xE0F0.
Proving this was easy, by adding an ASSERT(Ncb->RefCount >= 1) to the routine that dereferences NCBs. Obviously, we should not try to dereference an NCB that has a reference count of 0, because that NCB is now gone. Adding this ASSERT immediately caught the error, regardless of which pool implementation was being used, so this was a problem in ReactOS today, right now.
My first thought was that we were taking references to NCBs without incrementing the reference count. The NCB gets referenced in two places: when it gets created, and everytime a cached NCB is re-used for a new KCB (all this in CmpGetNameControlBlock).
After adding some tracing code, I discovered that CmpGetNameControlBlock would sometimes return an NCB that was cached, but without referencing it. I did not understand why, since the code says "if (Found) Ncb->RefCount++".
Further analysis showed that what would happen, on this particular instance, is that NCB "Foo" was being Found, but NCB "Bar" was returned instead. Therefore, causing some serious issues: First, NCB Foo was receiving too many references. Secondly, NCB Bar was not being referenced.
Worse though, it turns out this would happen when "Foo" was the CORRECT NCB, and "Bar" was an INCORRECT NCB. What do we mean by correct and incorrect? Well, because NCBs are hashed, it's possible for two NCB hashes to be VERY SIMILAR, but only ONE OF THOSE NCBs will be the right one -- for example, HKLM\Software\Hello vs HKLM\Software\Hell.
In our case, when a KCB for "Hello" was searching for the "Hello" NCB, the "Hello NCB would get a reference, but the "Hell" NCB would be returned. In other words, whenever a HASH COLLISION happened, the incorrect NCB was returned, probably messing up registry code in the process. Subsequently, when the KCB was dereferneced, it was attached to this incorrect, under-referenced NCB.
Since in ANY hash collision with "Hell", in our example, the "Hell" NCB would come first, subsequent searches for "Hellmaster", "Hellboy", "Hello World" would all still return "Hell". Eventually when all these KCBs would go away, the "Hell" NCB would reach even -18 references.
The simple solution? When the CORRECT NCB is found, STOP SEARCHING! By adding a simple "break" statement. Otherwise, even after the correct NCB is found, further, incorrect, collided NCBs are found, and eventually the last one ("Hell", in our example) got returned, and under-referenced, while "Hellmaster" and "Hellboy" were not returned, but LEAKED REFERENCES.
There you have it folks, MEMORY CORRUPTION (USE-AFTER-FREE), INCORRECT REGISTRY NAME PARSHING, REFERENCE LEAKS and REFERENCE UNDERRUNS, all due to ONE missing "break;".
-r
svn path=/trunk/; revision=47605
[NTOS]: Fix up NTAPI location in function definition.
[NTOS]: Implement even more stringent header checks: ExpCheckPoolHeader and ExpCheckPoolBlocks. Normally we would only want this on a DBG build, but I am enabling them for now until I can fix paged pool. If your machine crashes, reverting this commit is NOT the solution (boots for me).
[NTOS]: Add a AllowPagedPool BOOLEAN that will allow us to selectively enable when the ARM3 pool can be used, playing around with the situation that causes the corruption, and perhaps making it easier to find/fix.
svn path=/trunk/; revision=47600
[NTOS]: Enable them.
This boots on my system -- if it doesn't boot on yours, someone is corrupting your nonpaged pool. Reverting this patch is NOT the solution to your woes.
svn path=/trunk/; revision=47598
- Implement basic support for history in line editing
- Reorganize code to reflect that line input is more coupled to history than it is to character input
svn path=/trunk/; revision=47597
[NTOS]: Define POOL_BLOCK_SIZE definition to set the minimum pool block size. In NT, this is equal to a LIST_ENTRY structure, because the Pool Allocator must be able to store a LIST_ENTRY into a freed pool block. This also determines the alignment of pool allocations. So 8 on x86, 16 on x64.
[NTOS]: Don't depend on LIST_ENTRY, but use POOL_BLOCK_SIZE instead (on IA64, if we ever want to support this, the pool block size is different from a LIST_ENTRY/POOL_HEADER).
[NTOS]: The following ASSERTs must hold: the POOL_HEADER must be as big as the the smallest pool block (POOL_BLOCK_SIZE), which must be at least as big as a LIST_ENTRY structure. 8 == 8 == 8 on x86, 16 == 16 == 16 on x64.
svn path=/trunk/; revision=47592
NtDuplicateToken: Fail, if a primary token is to be created from an impersonation token and and the impersonation level of the impersonation token is below SecurityImpersonation.
svn path=/trunk/; revision=47586
[NTOS]: The paged pool free code was behaving incorrectly, assuming that paged pool was "locked down" and never paged out/reused (a valid NT operation mode), while the allocation code was assuming paged pool was a volatile, reusable, pageable resource (normal NT operation mode). The free code now assumes normal operation mode, and actually frees the freed paged pool pages, by using MiDeleteSystemPageableVm.
I have a feeling this will make ARM3 paged pool work.
svn path=/trunk/; revision=47582
- Implement console history (note: not too useful yet without any way to recall it)
- Implement APIs GetConsoleCommandHistoryLength, GetConsoleCommandHistory, ExpungeConsoleCommandHistory, SetConsoleNumberOfCommands, GetConsoleHistoryInfo, SetConsoleHistoryInfo.
- Remove stub of obsolete function SetConsoleCommandHistoryMode, which no longer exists in Windows.
svn path=/trunk/; revision=47580
[NTOS]: When expanding paged pool, initialize the PFN entry for the allocated page. Note we might be in arbitrary process space, so the PTE is not necessary valid for the process causing the expansion.
[NTOS]: Implement MiInitializePfnForOtherProcess to handle the case above.
[NTOS]: Change two static ASSERTs into C_ASSERTs. Might break non-x86 builds for a bit (vs breaking them at boot, which is worse).
Paged pool should start working soon.
svn path=/trunk/; revision=47579
- Use the old method for identifying the drive type (based on partition number) which actually works for floppies now because I changed the DrivePartition value returned (floppy = 0, cdrom = 0xFF) in a previous commit
- Fixes bug 5233
svn path=/trunk/; revision=47577
[NTOS]: Handle paged pool demand-zero fault fulfillment with MI_MAKE_HARDWARE_PTE macro.
[NTOS]: Use MiRemoveAnyPage instead of MmAllocPage, in paged pool demand-zero fault fulfillment.
These changes affect code paths that are not currently in-use.
svn path=/trunk/; revision=47575
- Put code for processing events for line input in one place, instead of duplicating it everywhere
- Remove "Fake" and "NotChar" fields from ConsoleInput struct. ConioProcessKey didn't actually add Fake events; they were used for the \n when converting \r to \r\n, but this is better done by the line input code.
- Build an input line completely on the server side; this will make it practical to add history and more sophisticated editing later
svn path=/trunk/; revision=47572
[NTOS]: Implement a MI_MAKE_HARDWARE_PTE macro for the generation of valid kernel PTEs instead of always taking the ValidKernelPte and changing its flags. This macro will take into account the protection mask (up until now ignored) and use the array previously implemented to determine the correct hardware PTE settings. Assertions are also added to validate correct usage of the macro, and later revisions will fill out NT-specific fields to help deal with transition PTEs, page faults, etc.
[NTOS]: Make the stack code the first user of this macro, for the stack PTEs. Good testing base as we create kernel stacks very often.
[NTOS]: The NT MM ABI specifies that in between the allocation of a new PTE and its initialization as a valid PFN, the PTE entry should be an invalid PTE, and should only be marked valid after the PFN has been initialized. For stack PTEs, do this -- first allocating the page, making it invalid, then initializing the PFN, and then writing the valid page.
svn path=/trunk/; revision=47571
- KSSTREAM_POINTER_OFFSET doesn't have an Alignment member on 64 bit systems. Comment the use out in these cases. It should probably be removed completely, as it's only an alignment / dummy value, but I leave this to the expert in this field.
- ULONG -> ULONG_PTR for pointer casts
svn path=/trunk/; revision=47567
- Move all amd64 specific files to one amd64 folder
- Compile x86 specific timer code only on x86
- Use KeRegisterInterruptHandler instead of manual idt manipulation
- add missing stubs for amd64
svn path=/trunk/; revision=47565
- Make Get/SetConsoleTitle more compatible with windows; in particular, transfer title via capture buffer to allow for longer titles.
- Tighten up capture buffer validation in win32csr.
svn path=/trunk/; revision=47562
