falahati / WindowsFirewallHelper

A class library to manage the Windows Firewall as well as adding your program to the Windows Firewall Exception list.
MIT License
276 stars 72 forks source link

Exception thrown: 'System.NotSupportedException' in WindowsFirewallHelper.dll #51

Closed josephnarai closed 2 years ago

josephnarai commented 3 years ago

I'm getting the following exceptions being thrown whenever I call any of the Firewall functions

Exception thrown: 'System.Runtime.InteropServices.COMException' in System.Private.CoreLib.dll Exception thrown: 'System.Runtime.InteropServices.COMException' in WindowsFirewallHelper.dll Exception thrown: 'System.NotSupportedException' in WindowsFirewallHelper.dll

So in this example:

            if (FirewallManager.IsServiceRunning)
            {
                try
                {
                    var activeProfile = FirewallManager.Instance.GetActiveProfile();
                    var rules = FirewallManager.Instance.Rules.ToList();

I'll get 3 sets of these exceptions thrown (one for each reference to FirewallManager

The strange thing is that it's all working fine, no other errors or issues.

I'd prefer to not have any exceptions being thrown... Is there a way to prevent it?

Thanks.

falahati commented 3 years ago

if this error is thrown for every call to the library, how do you mean that it works fine except for these exceptions? We have a TryGetInstance method that might be useful, I am thinking that maybe there is no firewall available on this device. FirewallManager.IsServiceRunning throwing an error is however unexpected and I will look into it.

josephnarai commented 3 years ago

As I said, all functions work as expected. IsServiceRunning returns true, activeProfile is correctly set (to Private in my test case) and rules comes back with a list of 530 odd rules. I just get these exceptions thrown whenever I access FirewallManager before the function or property returns.

As I said, it's all working ok, I would just prefer that there were not exceptions thrown every time I call it, as I'm running real time audio in the application.

falahati commented 3 years ago

so it works alright, the problem is that the library internally throws exceptions and then captures them. right?

well, I am sure that it is understandable that this library just like any other piece of code or program is created in multiple levels and lower levels might throw exceptions to notify the upper layer about a problem. I don't know if I can do anything about this, especially as you can see, some of these exceptions are thrown by the underlying net libraries.

but how does it affect your code?

  1. Does the exceptions that are thrown internally get captured while debugging the code? if this is the case, disable debugging of third-party libraries in VS debug config and/or disable break on these exceptions again via debug settings of VS.
  2. Is the problem with the performance hit of throwing an exception and then capturing it? meaning that it adds lag to the application? so in this case the solution is to not run these codes so many times to affect the performance of the application. I mean these are firewall commands after all and you should not have to run them every second or so, these maybe should be called once for a normal application unless the application's job is especially to manage the firewall which doesn't seem to be the case. you can also try moving the code to another thread, but keep in mind that COM objects are not thread-safe so you have to use a scheduler to perform operations on the firewall object.

If I got it wrong please provide more information about the exact problem and hopefully I might be able to suggest a workaround or fix the code if possible

josephnarai commented 3 years ago

I understand that the code is written relying on handling exceptions. Personally I avoid this technique when possible due to the performance hit. If there is an exception thrown in normal operation, I'll check and avoid the call causing the exception if at all possible. But I understand this is how it's written and can't be fixed.

I'm running the firewall check only on a background thread. Do you mean I need to schedule the call on the UI thread or something else? As I said it appears to be working fine as is - I don't change the firewall settings, only monitor them and report an error to the user if misconfigured.

One other unrelated question, are there any events generated for changes in firewall rules? Currently I have a background thread checking every 30 seconds... but this is a pretty ugly way of checking...

falahati commented 3 years ago

understandable.

the thing is that this library was never written to be light on operations since there is normally no need to do so as it is a firewall management library; and sure exceptions do have a great hit, mainly in debug and much less in release, but at the end of the day, if you look into the code closely you can see that we don't really throw that many exceptions anyway. For example, FirewallManager.IsServiceRunning is just a property using the ServiceController class. I don't know how I can decrease the number of exceptions for a code I don't even control.

The only place that we actually throw an exception is the COMTypeResolver class, but as you can see in the code there are a ton of conditions to check before trying to create an instance of a COM object, still tho, your information indicates that the first exception is thrown by the System.Private.CoreLib.dll, meaning that even with all this, sometimes we can not be sure about the existence of a type before actually trying to create an instance from it. If I can't really be sure if a type exists or not, I can not break from the function either. So at the end of the day, yeah I can probably remove one exception from here or there, but it won't help you at the slightest since you are not actually getting those exceptions at all, what you are getting seems to be from the underlying library that bubbles through different parts of the code.

You can however decrease the effect of these, for example, FirewallManager.IsServiceRunning can probably get cached and then revalidated on an exception if necessary. Active profile can also get cached. these two would decrease the performance hit by 60% by themselves.

In regard to using this function to detect changes to the rules; yes you are correct, this is not the right way to do so and this library also does not provide a better way for this problem. we have an open issue for this feature here but since it is not really related to the Firewall COM object, I have been hesitant to seriously look into it, also it probably will take a lot of my time which I don't have much of to spare currently. Maybe when I get a little less busy, I can look into this more closely. In any case, tho, any help or maybe a PR is greatly appreciated.

exception hit should also be contained in a single thread, so if you are running all your code in a background thread it should not affect your UI code. If it does, it is possible that the background worker is not on a separate thread compared to the one processing your audio, or that there is a hard coupling between the two. So it might help to look into how the code is executed, try to separate it into a separate, real thread (not a worker pool), try not to wait on the result of the said thread (use dispatched to just notify the other thread when something of interest happens, like when a rule changes) and try it on release profile. debug profile especially when called with VS open would result in the code getting slow as all threads are coupled with VS's debug engine to notify it of any exception. So when VS is running and your code is compiled in debug profile, it is possible that perfectly separated codes wait on each other simply because VS is trying to get information about an exception.

otherwise, I don't think that I can provide additional help here. but I will keep this issue open until I can take a close look into the code and see if there are ways to get around some of the exceptions without breaking things, if I ended up finding no way to do so, I will close it.

falahati commented 3 years ago

it seems that the FirewallManager.IsServiceRunning also relies on the version of the firewall, so COMTypeResolver is again at play here.

you might also want to cache the FirewallManager.Instance. it might solve some of your problems and I don't see any reason to not simply cache it. what seems to be the problem is that some of these properties should be methods to not confuse the user of the library, in this case, Instance property actually does more than you might expect (finding the right firewall instance). so caching it makes sense regardless of the fact that it is a property..

josephnarai commented 3 years ago

All good, thanks for taking the time to look into it for me.

Caching the firewall Instance helped. Those exceptions are now gone.

I can't seem to cache FirewallManager being a static class I guess...

I don't suppose there is way to get IsServiceRunning any other way?

falahati commented 3 years ago

Nah, unfortunately, there isn't a way to do so, but IsServiceRunning itself can be cached, it is quite unexpected for the service to stop in the middle of the application's execution (or start) so you are probably ok with running it only once to make sure firewall is actually active before getting into the loop. It is also a global property so you can run it on any thread you like. so maybe once on startup or something. then you can simply wait to see if an exception is thrown by .Rules (for any reason) and then see if the firewall service is active just to know what actually caused the exception. Don't really need to check it every time.

josephnarai commented 3 years ago

Interesting.

The way I'm using it is to inform the user if the firewall is blocking one of the applications we need for discovery, to pass audio and other control and feedback. So I feel like I do need to check if it's running in the case that the user has disabled the firewall altogether (in which case the rules are irrelevant and should not be checked).

Is the service still running even if the user disables the firewall? I have not checked that process yet to see what changes in what I'm returned!

josephnarai commented 3 years ago

So I checked and IsServiceRunning is still true even if I have disabled the firewall for each of the profiles... and the active profile is still returning as Private.

How do we determine if the firewall is off for a particular profile?

falahati commented 3 years ago

you can not actually stop the firewall service unless you have SYSTEM permissions and if the user reached that there seem to be an elevation issue with Windows, there isn't much you can do if the integrity of the OS is compromised like this.

you are probably more interested in the IsActive and Enable properties of each profile.

Enable is what you see on the firewall page that the user can affect.

Also, I am not really sure about this, but I expect that the COM object is exposed by the firewall service, so disabling the service in any way would simply result in all functions to throw an exception.

josephnarai commented 3 years ago

Great, I'll take a look at that.

Thanks again for your help.

This should result in a single set of exceptions on startup and then no further exceptions during normal operation :)

falahati commented 3 years ago

btw, you must use the group policy to limit the user's access to the firewall so that he cant disable it. I sense that this is a monitoring software in a corporate environment, usually in these cases, the access of the user is limited instead of checking the system to find modifications like this.

also, you can probably check the connection directly, try to see if you can ping the server instead of checking all firewall rules to see if it is blocked by a rule or something. I don't know about the requirements, but personally, based on what I have figured by now, I suspect that there could be better ways to achieve this

josephnarai commented 3 years ago

It's not a corporate environment. I had a case where a user installed the app and did NOT accept the firewall rule and then had to contact support. This implementation allows the error system in the app to report to the user that the firewall is misconfigured, this saving a call to support. It works very well. Thank you again for your excellent library :)