Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERROR: Caught unexpected exception while performing operation 'VirtualMachineOperation' #747

Closed
arielramos opened this issue Dec 9, 2023 · 18 comments
Assignees
Labels
bug This issue describes a bug in the software.
Milestone

Comments

@arielramos
Copy link

arielramos commented Dec 9, 2023

IMPORTANT
Please always consult the documentation first before creating a bug report!
https://safeexambrowser.org/windows/win_usermanual_en.html

Describe the Bug
When I try to execute SEB in Windows 10, the app does not start correctly, older versions work fine, but latest 3.6.0 not.

Steps to Reproduce
Steps to reproduce the behavior:

  1. Go to Moodle and start a new quiz attempt
  2. The SEB will crash

Expected Behavior
To start the quiz attempt with out any problems

Screenshots
Screenshot 2023-12-08 at 23 03 47

Version Information

  • Windows 10 Professional
  • SEB-Version 3.6.0

Additional Context

2023-12-06 21:09:10.587 [09] - INFO: Validating virtual machine policy...
2023-12-06 21:09:10.603 [09] - ERROR: Caught unexpected exception while performing operation 'VirtualMachineOperation'!

   Exception Message: Object reference not set to an instance of an object.
   Exception Type: System.NullReferenceException

   at SafeExamBrowser.SystemComponents.VirtualMachineDetector.IsVirtualSystem(String biosInfo, String manufacturer, String model) in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.SystemComponents\VirtualMachineDetector.cs:line 116
   at SafeExamBrowser.SystemComponents.VirtualMachineDetector.HasHistoricVirtualMachineHardwareConfiguration() in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.SystemComponents\VirtualMachineDetector.cs:line 158
   at SafeExamBrowser.SystemComponents.VirtualMachineDetector.IsVirtualMachine() in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.SystemComponents\VirtualMachineDetector.cs:line 60
   at SafeExamBrowser.Runtime.Operations.VirtualMachineOperation.ValidatePolicy() in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.Runtime\Operations\VirtualMachineOperation.cs:line 54
   at SafeExamBrowser.Core.OperationModel.OperationSequence.Perform() in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.Core\OperationModel\OperationSequence.cs:line 108

2023-12-06 21:09:10.606 [09] - INFO: ### -------------------------------------- Session Start Failed -------------------------------------- ###

2023-12-06 21:11:21.342 [09] - INFO: Initiating shutdown procedure...
2023-12-06 21:11:21.344 [09] - INFO: Stopping communication host...
@dbuechel
Copy link
Member

@arielramos Could you please post the entire log files? Line 116 as specified above cannot cause a NullReferenceException (under normal circumstances, we have seen weird stack traces under not-normal circumstances like VMs...), see https://github.com/SafeExamBrowser/seb-win-refactoring/blob/3.6.0/SafeExamBrowser.SystemComponents/VirtualMachineDetector.cs#L116.

@Notselwyn Do you see anything that could fail or lead to a NullReferenceException in the code checking for historic virtual machine configurations?

@dbuechel dbuechel added the information required This issue lacks information or requires feedback. label Dec 11, 2023
@Notselwyn
Copy link
Collaborator

A NullReferenceException should only be possible when biosInfo, manufacturer, and/or model is NULL I believe. The call is coming from

hasHistoricConfiguration |= IsVirtualSystem(biosInfo, (string) systemManufacturer, (string) systemProductName);

The biosInfo argument is defined as a format string, which means that it cannot be NULL.

This means that probably the registry keys accessed here do not exist:

success &= registry.TryRead(hardwareConfigKey, "SystemManufacturer", out var systemManufacturer);
success &= registry.TryRead(hardwareConfigKey, "SystemProductName", out var systemProductName);

Said registry keys are:

HKEY_LOCAL_MACHINE\SYSTEM\HardwareConfig\... -> SystemManufacturer
HKEY_LOCAL_MACHINE\SYSTEM\HardwareConfig\... -> SystemProductName

However, the return values are checked for function calls to registry.TryRead() on which the variables are depending. This leads to be believe there may be some malfunctioning of the return value of registry.TryRead() which is indicating the success of defining the output variable.

The return value indicating success only fails when an exception occurs. This gives me an idea: would it be possible that Microsoft.Win32.Registry.GetValue() does return NULL as value because the value name does not exist, yet does not trigger an exception. This is suggested by the docs @ https://learn.microsoft.com/en-us/dotnet/api/microsoft.win32.registry.getvalue?view=net-8.0 .

@dbuechel I suggest we implement this check:

		public bool TryRead(string key, string name, out object value)
		{
			var success = false;

			value = default;

			try
			{
				value = Microsoft.Win32.Registry.GetValue(key, name, default);
+				if (value != NULL)
					success = true;
			}
			catch (Exception e)
			{
				logger.Error($"Failed to read value '{name}' from registry key '{key}'!", e);
			}

			return success;
		}

@arielramos
Copy link
Author

Hi, thank you for your reply, here you have the logs.
2023-12-06_21h09m09s_Runtime.log
2023-12-08_22h30m51s_Client.log

@dbuechel
Copy link
Member

dbuechel commented Dec 12, 2023

@Notselwyn Yes, I think you're right, that is actually a bug in the code which we should fix. The current implementation will indeed return true with an output value of null if the name and/or the key do not exist. Ideally, we should thus first check whether the key and name do in fact exist and only then attempt to read the value, but I could not find an API that would allow to do so. Are you aware of one?

If there's no API, then we'd actually need to make the check a bit more elaborate:

public bool TryRead(string key, string name, out object value)
{
+	var defaultValue = new object();

	value = default;

	try
	{
		value = Microsoft.Win32.Registry.GetValue(key, name, defaultValue);
	}
	catch (Exception e)
	{
		logger.Error($"Failed to read value '{name}' from registry key '{key}'!", e);
	}

+	return value != default && value != defaultValue;
}

I assumed that the actual value of a name can be null (e.g. for a binary or string value type) and hence did not return value != default, but scanning the actual framework implementation suggests otherwise: https://referencesource.microsoft.com/#mscorlib/microsoft/win32/registrykey.cs,c67ba10c47948465.

Would you agree that the above change in fact eliminates the bug and ensures that if we return true, then the output value will never be null? Or can there in fact be registry names with a value of null, which in turn would prevent us from checking for value != default?

@dbuechel
Copy link
Member

dbuechel commented Dec 12, 2023

@arielramos I still cannot explain your stack trace, that particular line of code cannot cause a NullReferenceException (it should, in case our analysis is correct, happen on line 119 or 120):

2023-12-06 21:09:10.603 [09] - ERROR: Caught unexpected exception while performing operation 'VirtualMachineOperation'!

   Exception Message: Object reference not set to an instance of an object.
   Exception Type: System.NullReferenceException

   at SafeExamBrowser.SystemComponents.VirtualMachineDetector.IsVirtualSystem(String biosInfo, String manufacturer, String model) in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.SystemComponents\VirtualMachineDetector.cs:line 116
   at SafeExamBrowser.SystemComponents.VirtualMachineDetector.HasHistoricVirtualMachineHardwareConfiguration() in C:\Users\appveyor\projects\seb-win-refactoring-phv3mbelx16fm9ou\SafeExamBrowser.SystemComponents\VirtualMachineDetector.cs:line 158
   [...]

Can you explain why there is no system information in the runtime log file resp. what computer you are using (manufacturer and model)?

# Computer 'WRR' is a                                   manufactured by                                 

@Notselwyn
Copy link
Collaborator

@Notselwyn Yes, I think you're right, that is actually a bug in the code which we should fix. The current implementation will indeed return true with an output value of null if the name and/or the key do not exist. Ideally, we should thus first check whether the key and name do in fact exist and only then attempt to read the value, but I could not find an API that would allow to do so. Are you aware of one?

That is indeed the best approach. I haven't looked into the semantics but perhaps open a registry key, use RegistryKey.GetValueNames(), validate the value name, and then get the value? The obvious problem with this is that there's quite a bit of overhead, so the approach depends on how frequent the TryRead() function is called in the codebase.

Would you agree that the above change in fact eliminates the bug and ensures that if we return true, then the output value will never be null? Or can there in fact be registry names with a value of null, which in turn would prevent us from checking for value != default?

I believe this would interfere when registry values overlap with default values for types. I.e. if a registry value type is int and the registry value is 0, then the return value may indicate failure considering registry value 0 is the default value. If we take this approach I recommend using the fact that an existing registry value should not be NULL:

public bool TryRead(string key, string name, out object value)
{
+	var raw_value = NULL;  // set to NULL for fallback when exception happens

	try
	{
+		raw_value = Microsoft.Win32.Registry.GetValue(key, name, null);
	}
	catch (Exception e)
	{
		logger.Error($"Failed to read value '{name}' from registry key '{key}'!", e);
	}
	
+	if (raw_value == NULL)
+       {   
+           value = default;
+           return false;
+       }

+       value = raw_value;
+       return true;
}

(I'm using a separate var here which could be NULL, so the value out arg will never be set to NULL considering I don't know out of the top of my head if it can be )

@dbuechel
Copy link
Member

dbuechel commented Dec 12, 2023

Yes indeed, I think there is no way around actually trying out different approaches. I think value != default should work, as value is defined as an object reference and not a value type like e.g. int, but here again, I think the best would be to try it out and see what works (best). I shall take this up in our internal planning and tackle it next year.

@arielramos
Copy link
Author

Hi, these are the computer specs:

Intel Core i7 2700K
Intel motherboard, DZ68BC LGA115 socket ATX form Z68 Express chipset
OCZ ZX Series 850W Fully-Modular 80PLUS Gold High Performance Power Supply
Antec DF-85 Black ATX Full Tower Computer Case
16GB (4 x 4GB) 240-Pin DDR3 SDRAM DDR3 1600 (PC3 12800)
Windows 7 Professional, upgraded to Windows 10 Professional
C:\ 1 TB SSD for programs 792 GB free
I:\ 1 TB SSD for data 480 GB free

Windows, BIOS, Norton updates all current.
Use Chrome updates current.
Also have Edge and Mozilla/FoxPro updates current

@arielramos
Copy link
Author

@dbuechel
Copy link
Member

Hi, these are the computer specs:

Intel Core i7 2700K Intel motherboard, DZ68BC LGA115 socket ATX form Z68 Express chipset OCZ ZX Series 850W Fully-Modular 80PLUS Gold High Performance Power Supply Antec DF-85 Black ATX Full Tower Computer Case 16GB (4 x 4GB) 240-Pin DDR3 SDRAM DDR3 1600 (PC3 12800) Windows 7 Professional, upgraded to Windows 10 Professional C:\ 1 TB SSD for programs 792 GB free I:\ 1 TB SSD for data 480 GB free

Windows, BIOS, Norton updates all current. Use Chrome updates current. Also have Edge and Mozilla/FoxPro updates current

Thanks a lot. I presume you have a custom build which could explain the missing manufacturer and model name in the system info.

@dbuechel
Copy link
Member

dbuechel commented Dec 13, 2023

Also I have these new log files [...]

As the client log file shows, you were running a prohibited application and SEB was unable to terminate it automatically:

2023-12-12 20:38:39.489 [01] - ERROR: Failed to automatically terminate application 'remoting_host.exe'!

@arielramos
Copy link
Author

Thank you for all your help

Does that mean that an improvement can be made to the code to take into account people who have computers they build themselves?

I'll check the remoting_host.exe

@dbuechel
Copy link
Member

Yes, we'll definitely improve the code as outlined above. You'd need to test the beta build once the changes are implemented in order to see whether these indeed fixed the issue, as the stack trace you posted still seems inexplicable to me. But that last line might just be an aftereffect of an earlier NullReferenceException, though that would be very big news to me (up and until today, every stack trace I've ever seen has been accurate).

@arielramos
Copy link
Author

Hi, I have an apps called LogMeIn as a remote desktop, but, I closed the app in the task manager and then use the SEB, but issue persists.
2023-12-13_10h59m30s_Runtime.log
2023-12-13_10h59m30s_Client.log
2023-12-13_10h59m30s_Browser.log

I attach other logs.

@dbuechel
Copy link
Member

Yes, that is still the same issue and I'm afraid you won't get around it until we implemented a fix (which is currently planned for January). If it is possible, try to downgrade and use SEB 3.5.0 in the meantime.

@dbuechel dbuechel added bug This issue describes a bug in the software. and removed information required This issue lacks information or requires feedback. labels Dec 14, 2023
@dbuechel dbuechel added this to the 3.7.0 milestone Dec 14, 2023
@dbuechel
Copy link
Member

@arielramos Could you please test the latest beta build with the changes from @Notselwyn: https://sebdev-let.ethz.ch/api/buildjobs/c1928d85u6cyhqyn/artifacts/SEB_3.7.0.647_SetupBundle.exe.

dbuechel added a commit that referenced this issue Jan 15, 2024
@arielramos
Copy link
Author

Hi @dbuechel , hope you are well, we downloaded the beta and now is working, thank you so much, we really appreciate your effords.

Regards

@dbuechel
Copy link
Member

Excellent, thanks for testing and the timely feedback, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a bug in the software.
Projects
None yet
Development

No branches or pull requests

3 participants