Help diagnosing a problem with cygwin/samba on ROS

All development related issues welcome

Moderator: Moderator Team

bugdude
Posts: 22
Joined: Wed Nov 27, 2019 11:41 pm

Re: Help diagnosing a problem with cygwin/samba on ROS

Post by bugdude »

Didn't mean to annoy you. I am working on building the test you sent to see if I can reproduce the failure with it. I did extract some code out of Cygwin that shows the pattern used in the code, but I'll hold onto that until I can reproduce the failure with the test.

bigdude
bugdude
Posts: 22
Joined: Wed Nov 27, 2019 11:41 pm

Re: Help diagnosing a problem with cygwin/samba on ROS

Post by bugdude »

learn_more,

The reason your test code does not fail is that you re-used the same structures for both calls to NtOpenFile, while in my Delphi test app I declared two independent structures for ObjectAttributes, iosb, and FileStandardInformation. It seems that if a string is already present in the structure and you try to replace it with an empty constant InitializeObjectAttributes leaves the original string in place. I could be wrong, but it seems you may have hit a completely separate issue with the macro for InitializeObjectAttributes. Since I used two separate structures, I get different results which includes triggering the failure condition I am seeing in Cygwin's NtOpenFile usage. In Cygwin the calls come from separate functions with local structures in different scopes so the structures cannot be the same ones. If you alter the test code to use two independent structures for the calls to NtOpenFile you will recreate the failure condition with your test code. I did that, and added some instrumentation to print the filesizes and with those changes your test code got the exact same failed results I see in Delphi/Cygwin including the second handle being the volume (distinguishable by its size).

I continued to work on building a patch for the issue and I came up with one that seems to work. I am testing it more carefully now, but it comes down to inserting an 'if' to detect the parameter condition and execute a separate code branch that uses ObReferenceObjectByHandle and ObDuplicateObject to duplicate the handle received in ObjectAttributes->RootDirectory. The code below would be inserted just after the parameter validations in the IopCreateFile function in iomgr\file.c. As I mentioned before I am not a C programmer so I'm sure it requires cleanup and validation to really be usable once the DPRINT1 instrumentation is converted to match the style used in the rest of file.c but the code builds and works for the tests I've run using your test code and my and Delphi test. I will test it with my preliminary Samba build shortly. There is probably a cleaner way to detect the empty unicode_string than the one I used, but those are not friendly to work with..

Code: Select all

    /* Handle Cygwin edge case of re-opening FileObject by calling NtOpenFile passing only existing Handle. Deflect the call to 
       ObDuplicateObject because we don't need OpenPacket or other calls in this case, we have all we need in hand already. */

    if ((ObjectAttributes->ObjectName->Length <= 1 || (!ObjectAttributes->ObjectName->Length) || (!ObjectAttributes->ObjectName) || 		 
        (!ObjectAttributes->ObjectName->Buffer)) && (ObjectAttributes->RootDirectory != 0) && (ObjectAttributes->RootDirectory != NULL)) 
    {
    	Status = ObReferenceObjectByHandle(NtCurrentProcess(), 
                                       	   PROCESS_DUP_HANDLE,
                                       	   PsProcessType,
                                       	   PreviousMode,
                                       	   (PVOID*)&SourceProcess,
                                       	   NULL);
    	if (!NT_SUCCESS(Status)) {
        	DPRINT1("(PID %lx) IopCreateFile call to ObReferenceObjectByHandle for Source Process failed - status: %lx\n", 
        	                                         PsGetCurrentProcessId(), Status); 
		return Status;
    	}
    	Status = ObDuplicateObject(SourceProcess, 
                               	   ObjectAttributes->RootDirectory,
                               	   SourceProcess, 
                               	   &LocalHandle,
                               	   DesiredAccess,
                               	   ObjectAttributes->Attributes,
                               	   CreateOptions,
                               	   PreviousMode);
    	if (!NT_SUCCESS(Status)) {
        	DPRINT1("(PID %lx) IopCreateFile call to ObDuplicateObject failed - status: %lx\n", PsGetCurrentProcessId(), Status); 
        	ObDereferenceObject(SourceProcess);
		return Status;
    	}
    	ObDereferenceObject(SourceProcess);

    	if (AccessMode != KernelMode) 
    	{ /* probe for write access for non-kernel mode */
	       	_SEH2_TRY
        	{
            		/* Probe the output parameters */
            		ProbeForWriteHandle(FileHandle);
            		ProbeForWriteIoStatusBlock(IoStatusBlock);
        	}
        	_SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
        	{
            		/* Get the exception status */
            		Status = _SEH2_GetExceptionCode();
        	}
        	_SEH2_END;
    	}   

	/* perform write back of handle and status information */
        _SEH2_TRY
        {
            	*FileHandle = LocalHandle;
            	/* IoStatusBlock->Information = OpenPacket->Information; */
            	/* since we do not use Io for this calling mode set up 'fake' Io status and final status if we didn't crash before this point we succeeded */
            	IoStatusBlock->Status = STATUS_SUCCESS;
            	Status = STATUS_SUCCESS;
        }
        _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER)
        {
            	/* Get the exception status */
            	Status = _SEH2_GetExceptionCode();
        }
        _SEH2_END;

    	if (!NT_SUCCESS(Status)) {
       		if (LocalHandle != 0) {
          		DPRINT1("(PID %lx) IopCreateFile Completed status != NT_SUCCESS - returning status: 0x%lx handle: %p FileName: %wZ \n", 
                                                                          PsGetCurrentProcessId(), Status, LocalHandle, ObjectAttributes->ObjectName );
       		} else {
          		DPRINT1("(PID %lx) IopCreateFile Completed status != NT_SUCCESS - returning status: 0x%lx handle: 0x0 FileName: %wZ \n", 
                                                                          PsGetCurrentProcessId(), Status, ObjectAttributes->ObjectName );
       		}
    	}
    	return Status;
    } /* cygwin edge case code branch ends here */
Bugdude
hbelusca
Developer
Posts: 1196
Joined: Sat Dec 26, 2009 10:36 pm
Location: Zagreb, Croatia

Re: Help diagnosing a problem with cygwin/samba on ROS

Post by hbelusca »

Bear in mind that Windows doesn't have a "Handle Cygwin bugs" case in its kernel, so your "patch" is most probably wrong.
bugdude
Posts: 22
Joined: Wed Nov 27, 2019 11:41 pm

Re: Help diagnosing a problem with cygwin/samba on ROS

Post by bugdude »

That is true, but neither Cygwin or the test applications fail under windows when they make the same calls, so internally Windows handles this case very differently than ReactOS currently does. Thank you for your help, the test that you sent makes it possible to recreate the error using ROSBE based code so it should make the case much clearer.

I reported this on JIRA a moment ago because it is what most would define as bug since ROS does not match the behavior of W2K3 or XP. It seems to be a rarely occurring issue, but it could still be impacting other applications preventing them from working on ROS as well. I attached a modified version of the test you sent me and the preliminary patch to the JIRA report as well. For my purposes I now have the patched build that I can use to continue to test the Samba build I have been working on.

I have had to create small patches for Samba, Cygwin, waf, and Python as well as make lots of changes to the Samba build system to get those to even build. It will sound strange to say it, but I am getting used to finding issues everywhere in the stack on this pet project. In fact hunting this quirk down in ROS has been like the rest of the project, it has been an educational brain twister.

Bugdude
Post Reply

Who is online

Users browsing this forum: No registered users and 1 guest