Core ReactOS

Reviews in Crucible

Shows reviews for code changes to Core ReactOS.

Filter reviews:
CR-101 Under Review

Font selection

CR-98 Under Review

GSoC USB project

CR-94 Closed

CString implementation

This CR aims to implement most basic CString operations.
Please note, the patch is not finished (see all commented out items in CString.inl)

CR-93 Closed

xml2sdb - build tool

This host-tool will convert xml files (like the one included) into an Sdb database.

CR-92 Under Review

GSoC AHCI Project

CR-91 Closed

Free stack before calling recursive noreturn handler in HalpEndSoftwareInterrupt

HalpEndSoftwareInterrupt has an optimization where it calls the noreturn version of a software interrupt handler instead of looping. This works because that handler will in turn call Hal(p)EndSoftwareInterrupt and continue the cycle.
In the event of a high frequency interrupt queueing a DPC, this can lead to the following stack in ROS:
809def68 <NTOSKRNL.EXE:81bcd (ntoskrnl/ke/bug.c:1100 (KeBugCheckWithTf))>
809def88 <NTOSKRNL.EXE:11fdaf (ntoskrnl/ke/i386/exp.c:1144 (KiSystemFatalException))>
809def90 <NTOSKRNL.EXE:123e63 (ntoskrnl/ke/i386/traphdlr.c:854 (KiTrap08Handler))>
809def98 <NTOSKRNL.EXE:335e (:0 (KiTrap08))>
f77e90a0 <NTOSKRNL.EXE:11f43a (ntoskrnl/ke/i386/exp.c:621 (KeTrapFrameToContext))>
f77e9428 <NTOSKRNL.EXE:11f7df (ntoskrnl/ke/i386/exp.c:870 (KiDispatchException))>
f77e94a0 <NTOSKRNL.EXE:11fd73 (ntoskrnl/ke/i386/exp.c:1126 (KiDispatchExceptionFromTrapFrame))>
f77e94d0 <NTOSKRNL.EXE:122e11 (ntoskrnl/ke/i386/traphdlr.c:229 (KiDebugHandler))>
f77e94e8 <NTOSKRNL.EXE:124eb4 (ntoskrnl/ke/i386/traphdlr.c:1593 (KiDebugServiceHandler))>
f77e94f0 <NTOSKRNL.EXE:3ba1 (:0 (KiDebugService))>
f77e956c <NTOSKRNL.EXE:13f854 (:0 (DebugService))>
f77e958c <NTOSKRNL.EXE:13f88c (lib/rtl/debug.c:28 (DebugPrint))>
f77e983c <NTOSKRNL.EXE:13fa5e (lib/rtl/debug.c:138 (vDbgPrintExWithPrefixInternal))>
f77e985c <NTOSKRNL.EXE:13fb50 (lib/rtl/debug.c:211 (DbgPrint))>
f77e988c <scsiport.sys:268f>
f77e98b4 <NTOSKRNL.EXE:11ff9b (include/crt/mingw32/intrin_x86.h:94 (KiInterruptDispatch))>
f77e98c4 <NTOSKRNL.EXE:1203be (ntoskrnl/ke/i386/irqobj.c:315 (KiInterruptTemplateHandler))>
f77e98c8 <f77e996c>
f77e996c <NTOSKRNL.EXE:12161a (include/crt/mingw32/intrin_x86.h:1574 (KiSwapContextExit))>
f77e99bc <NTOSKRNL.EXE:28bc (:0 (KiSwitchThreads))>
f77e99d4 <HAL.DLL:a634 (hal/halx86/up/pic.c:1328 (HalpDispatchInterrupt2ndEntry))>
f77e99ec <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77e9a04 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77e9a1c <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77e9a34 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77e9a4c <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77e9a64 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77e9a7c <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77e9a94 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77e9aac <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77e9ac4 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77e9adc <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77e9af4 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77ebbac <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77ebbc4 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77ebbdc <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77ebbf4 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77ebc0c <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77ebc24 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77ebc3c <HAL.DLL:a598 (hal/halx86/up/pic.c:780 (HalpEndSoftwareInterrupt))>
f77ebc54 <HAL.DLL:a647 (hal/halx86/up/pic.c:1346 (HalpDispatchInterrupt2ndEntry))>
f77ebc64 <HAL.DLL:a7a7 (hal/halx86/up/pic.c:1247 (HalEndSystemInterrupt))>
f77ebc8c <NTOSKRNL.EXE:11ffb3 (ntoskrnl/ke/i386/irqobj.c:144 (KiInterruptDispatch))>
f77ebc9c <NTOSKRNL.EXE:1203be (ntoskrnl/ke/i386/irqobj.c:315 (KiInterruptTemplateHandler))>
f77ebca0 <f77ebd5c>
f77ebd5c <NTOSKRNL.EXE:d4c23 (ntoskrnl/mm/ARM3/zeropage.c:102 (MmZeroPageThread))>

Each recursive call takes up a little bit of stack space, leading to a stack overflow. This fixes that situation by making an assembly wrapper around the C version of HalpEndSoftwareInterrupt, and freeing the stack there in case of a noreturn call.

CR-90 Under Review

Preliminary NTFS write support

Implement preliminary, with limited features, write support to NTFS.

CR-89 Closed

Improve delayload support

CR-88 Closed

Improve delayload support - add delayload apitest

CR-87 Under Review

Patches to fix 3dtext screensaver and improvements to desk.cpl
After applying them most issues are fixed. (There are still some redraw issues in the preview window, but that is minor)
Patch is by kostinio (shkil-k at mail dot ru)

CR-86 Under Review

Implement partial support for job objects

CR-83 Closed

Implement apphelp sdb layer

CR-82 Closed

Add Matt Wu ext2 filesystem driver

CR-81 Under Review

Hal::dma - fully implementation of S/G functions, correct check is EISA dma available

CR-79 Closed

ntoskrnl: implement NtApphelpCacheControl

Kernel-mode part of AppHelp stuff, including tests + a tool for debugging AppHelp.

CR-78 Closed

Generate XDK headers during build

CR-75 Closed

NTVDM DOS Device Support

CR-73 Closed

Intrinsics stuff...

CR-70 Closed

schannel implementation on top of mbedTLS

Let's create a review with this patch from CORE-9065

CR-69 Closed

Rewrite NtUserSetCursorIconData

CR-66 Under Review

Dynamic window resize for Virtualbox

Timo asked for a Crucible review, well, here it is!

CR-58 Closed

Support PAGE_NOACCESS in NtProtectVirtualMemory

The goal of this patch is to make VirtualProtect and friends useful for applications.

This is my first attempt to use transition PTEs. The code is quite ugly and certainly needs improvement, but it's functional.

I tested it successfully with firefox (Javascript compiler) and test from r63678.

The code supports the following scenario:
 * Allocate memory using VirtualAlloc.
 * Page it in with some write.
 * Mark it as NOACCESS using VirtualProtect
   ** PTE is made invalid, marked as transitional with the relevant protection mask.
   ** PFN share count is decreased and added to the Modified Page List if it reaches zero.
 * Raise exception if accessed.
 * Mark the memory as accessible again using VirtualProtect
   ** PTE is left as transition and protection mask is accordingly modified.
 * On access, the transition bit is removed and the corresponding PFN share count increased. PFN is marked as Active and valid again

Some comments:
 - Transition PTEs are not used anywhere in ARM3 (yet). I used the available infrastructure as far as I could, and code that I added could certainly be turned into macros/functions to be used anywhere else
 - There is no support for writing content of the pages back to HDD. For now, this is OK because ARM3 only has Virtual Memory and pagedfile-backed sections (without writing into it...), but this needs to be alleviated soon.

Side notes:
 - Aleksandar (TheFlash) expressed interest to work into MM. I included him in the review. Be gentle with him ;-)
 - Amine knows why you should consider this as "high priority" (cough!)

CR-56 Closed

NtSaveKey implementation

CR-54 Closed

About the idle thread stack / read-only segments in the Kernel

This patch adds guard pages to the startup stack and double fault stack. It does not increase the stack size. And it does not fix the problem that the kernels read-only sections are read/write.

CR-52 Closed

Implement renaming support in FastFAT

This patch fully implements support for renaming into FastFAT FSD (for both FAT & FATX) and removes huge hack in MoveFileWithProgressW() (kernel32.dll).

Here is the full changelog (aka commit message to be):
- Implement vfatPrepareTargetForRename() that prepares for renaming. It tries to open target and deletes it if it exists and if allowed. And then, it opens the parent directory.
- Implement VfatSetRenameInformation() that actually does the renaming (call as SetInformationFile). It handles cases we we have (or we don't have) TargetDirectory provided. It sends notifications as appropriated on demands.
- Implement vfatRenameEntry() that renames an entry in place. So far, it only supports FATX entries renaming. FAT entries are a bit more complex. It falls back to vfatMoveEntry() in later cases.
- Implement VfatMoveEntry() that will move an entry accross directories (or in place for FAT). Its principles are simple: it deletes the entry in old parent, and recreate it in new parent, keeping file metadata & data.
- Modify VfatDelEntry() and VfatAddEntry() so that they can handle deleting an entry without touching its data and adding an entry with an already provided FCB and thus use the given metadata.
- Implement vfatDelFCBFromTable() which is just old code moved to new routine to allow reuse. It deletes a FCB entry from hash table. Doesn't deal with references!
- Implement vfatMakeFullName() which is mostly old code moved to new routine to allow reuse. It allocates buffer and copy data for FCB full name.
- Implement vfatUpdateFCB() that will update a FCB with new names and parent. It will remove anything related to old name and will recreate using new data. It will adjust references count.
- Modify vfatMakeFCBFromDirEntry() so that it calls vfatMakeFullName().
- Modify vfatReleaseFCB() so that it calls vfatDelFCBFromTable().
- Revert VfatOpenFile() to its previous features.
- Modify VfatCreateFile() to reimplement support for SL_OPEN_TARGET_DIRECTORY. It is way less hackish than previously. It also properly opens parent now, by incrementing its handle count and by setting appropriate access rights.
- Rewritten MoveFileWithProgressW() to implement all the missing features that are used in Windows 2k3 including links and reparse points.
- Implemented BasepMoveFileDelayed() to replace deprecated add_boot_rename_entry(). This functions is matching the features implemented in SMSS.
- Implemented BasepMoveFileCopyProgress() which is used in MoveFileWithProgressW().
- Stubbed BasepNotifyTrackingService() which is not use at the moment (FastFAT, even in Windows doesn't provide such feature).
- Reimplemented ReplaceFileA(), MoveFileWithProgressA() to quit Winisms and use our internal helpers.
- Make MoveFileX() use MoveFileWithProgressX() directly.
- Fixed a few prototypes.

It anyway comes with a bug I can't fix atm (hence my review, I need your help guys :-)). To reproduce the bug, run kernel32_winetests:change. It will fail at deleting last directory "boo". I left my debug messages where you see that on deletion, that directory still has 7 references! Which sounds clearly as a reference leak, I just don't see where it comes from. Help!
If you run ntdll_winetests:change afterwards, it will attempt to browse such "deleted" directory to open it. Which will lead to a null pointer dereference (associated file object).
Otherwise, the features were successfully tested.

Updated on the 25/05/14: Added the second version of the patch, which optimizes a bit the renaming by reducing the number of FCB researches and takes into account the remark of Christoph.
If you run kernel32_winetests:change, you'll get 3 more failures (2 because if fails moving files) than with actual renaming. No regression on ntdll_winetests:change side.
Any help still welcome to get rid of such failures :-).

Updated on the 28/08/14: Added the third version of the patch. This one is pretty major because it takes into accounts feedbacks from Hervé regarding Kernel32 side and completely whips out old and deprecated code with w2k3 compatible code.
There is still 2 more failures in kernel32_winetests:change. Likely due to a reference count leak in fastfat (still & always :-().

Thanks in advance for your help!

CR-48 Closed

CORE-8098 - Fix for user32:menu crashes

IntGetMenuItemByFlag's failure-case behavior is extremely inconsistent, and much more so the way callers handle this. This results in a fair amount of uninitialized memory being used.

CR-40 Closed

Pushing drag and drop, context menu, and other copy paste operations to background threads for increased responsiveness of the shell

Basic code check. Need to ensure that there's no memory corruption or leaks.

Code be Frontier.

CR-33 Abandoned

CORE-6676: usbuhci: Invalid pointer dereference when accessing USB thumb drive

I've forwarded a USB disk drive to QEMU. It was successfully recognized and shown in explorer, but when accessing the drive (i.e. double clicking), usbuhci caused a page fault.
Log with backtrace and data interpretation attached.

CR-31 Closed

CORE-7105: atl: Simplify CComObject code, fix warnings

CR-30 Closed

PnP start fix (services)

EDIT: Changes committed in revision r58856.

Check that this commit doesn't break other things, before merging it into the 0.3.15 branch.

EDIT: Now that Ziliang committed in the 0.3.15 branch the r58534 code, these improvements will be FOR 0.4 !!

See and too.

- It seems (after testing) that services report now correctly their state to the SCM. So we can start them in SERVICE_START_PENDING state (see revisions r45626 and r45640).
- Add some informative comments.
- Use a helper function to create start events at initialization time.
- When autostart services are up, signal an event. (see revisions r45633 and r45658).
- Wait for LSASS just after having created the services database, and before calling ScmGetBootAndSystemDriverState (conform to Windows Internals 4th, page 224).


- When starting auto-start services, hold a lock during all the operation in such a way that, if an external program wants to start a service, it is obliged to wait till all the auto-start services have been started (usual service starting also uses that lock).

CORE-7001 #resolve #comment Should be fixed by r58534. Do not hesitate to reopen this bug-report if the problem reappears.

CR-29 Closed

0.3.15: cmd: ECHO command should write CRLF

CR-28 Closed

0.3.15: user32: Don't access past the file size when loading a cursor/icon

Fix for CORE-7027. Candidate for inclusion into 0.3.15.

CR-18 Closed

Simplify code for ExpLookupHandleTableEntry

CR-16 Closed

CORE-6822 Patch review

CR-15 Closed

shell32 - Start Menu implementation

This is a review of the shell32 start menu patch from Katayama Hirofumi MZ.
See for more information.

(The patch number 6 reviewed there corresponds to the number 5 -- see Jira -- but without the modified properties of the new added restart and shutdown icons).

CR-7 Under Review

reg query implementation

CR-5 Closed

Coverity fixes for Ntoskrnl

Fix almost all the Coverity code defects in the kernel.
See for more details.