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

[BUG] Writing data to System32 #120

Closed
Oscillate18 opened this issue Feb 25, 2023 · 23 comments
Closed

[BUG] Writing data to System32 #120

Oscillate18 opened this issue Feb 25, 2023 · 23 comments
Labels
bug Something isn't working

Comments

@Oscillate18
Copy link

Describe the bug
Program attempts to write the settings and DLSS files to the system32 directory, which fails without admin access. Launching with admin access writes these files to system32 and the DLSS downloads normally again.

Using the portable version, unsure if it is the same with the installer.

Expected behavior
Should not write these files to this directory.

Screenshots
Screenshot_20230225_060145
Screenshot_20230225_060154

@Oscillate18 Oscillate18 added the bug Something isn't working label Feb 25, 2023
@beeradmoore
Copy link
Owner

👀 100% should not try save StoredData there.

It should be using directory where the DLSS Swapper exe is located. Thanks for reporting

@Oscillate18
Copy link
Author

👀 100% should not try save StoredData there.

It should be using directory where the DLSS Swapper exe is located. Thanks for reporting

No worries, hopefully it's an easy fix.

It's an amazing program and very well made. Looking forward to future updates!

@beeradmoore
Copy link
Owner

Tried to replicate and struggled to get it to behave the same. Can you give me any info on what version of Windows you are using? Also are you just running the exe from the extracted folder, or are you running it directly from the zip, did you create a shortcut to it perhaps, or are you running as admin?

@Oscillate18
Copy link
Author

Tried to replicate and struggled to get it to behave the same. Can you give me any info on what version of Windows you are using? Also are you just running the exe from the extracted folder, or are you running it directly from the zip, did you create a shortcut to it perhaps, or are you running as admin?

So today it appears to be working normally again, nothing has changed since I last tried it. I have tried retracing the things I did in the program and could not get the same behaviour again.

I was running the exe from the extracted folder without a shortcut and only ran it as admin to see if it would write the data there, which it did. I did not run it as admin before this issue appeared. Running Windows 11, 22H2.

If it happens again, does the program create a log anywhere I can send you?

Go ahead and close the issue if you'd like since I can't get it to happen again and I'll report it again if it does.

@rennr
Copy link

rennr commented Mar 11, 2023

Happened to me too. I only got it to do it if I ran it from the start menu (like I searched for it using start and ran it like that). If I ran the exe from the download folder it did not cause an issue.

I suggest just downloading to %temp% rather than the current folder. It's possible the "current folder" is not the exe path if you run it from the start menu, so that's why it tries to write to system32.

@Oscillate18
Copy link
Author

Happened to me too. I only got it to do it if I ran it from the start menu (like I searched for it using start and ran it like that). If I ran the exe from the download folder it did not cause an issue.

I suggest just downloading to %temp% rather than the current folder. It's possible the "current folder" is not the exe path if you run it from the start menu, so that's why it tries to write to system32.

Can confirm, immediately happened again when running it from a search in the windows start menu.

It's normal when ran through the exe in the folder or through a shortcut.

@beeradmoore
Copy link
Owner

@Oscillate18 , I too am on Win11 22H2. I don't want to close it as it was and still is a problem. I don't think I can do any harm by failing (or successfully if running as admin) to write files there. It's just very not ideal.

I added an alert on startup to show where it was going to try write files. I made the folder C:\dlss_portable\ and made sure it was indexed, built a portable version of DLSS Swapper with the above change and put it in there.

I then tried:

  • launch it by searching it in the start menu
  • pinning DLSS Swapper from right mouse click context when looking at this folder
    Both of those worked fine.

image

I also tried running from a zip opened in explorer which is the default behavior and that failed to run at all.

@rennr , I tried %temp% and it worked as expected. However this did lead me to looking into why system32. I found this result talking about it. Seems that if a specific process tries to launch DLSS Swapper (eg start menu which would be Explorer.exe) it may set that processes working directory to the new DLSS Swapper process. OR if it does not set it by passing null it may set it to system32 by default.

I am using the current working directory to store files for portable app so it would make sense why it would store in system32, just now how it got into that state.

I can change the code to not use working directory and instead use the path the executable is in but I have seen people also have trouble with that. I'd prefer if I could replicate the problem before I fix it. But I could try the change, put it out as a pre-release portable package only and see if that fixes it.

Do either of you still have 1.0 portable failing? If you have 1.0 portable failing and you swap to 1.1 pre-release in the same sort of config and it works then we can assume then the fix was correct.

@Oscillate18
Copy link
Author

@beeradmoore

It only fails and writes to system32 when launched through search, same as rennr. It's completely normal again if ran through the exe in the folder or a shortcut.

I'd be happy to test a pre-release for you and see if it's still an issue.

@rennr
Copy link

rennr commented Mar 11, 2023

I can change the code to not use working directory and instead use the path the executable is in but I have seen people also have trouble with that. I'd prefer if I could replicate the problem before I fix it. But I could try the change, put it out as a pre-release portable package only and see if that fixes it.

I don't see an issue with using %temp%. The user's temp path should always be writable no matter who is running it. I would suggest using Path.GetTempPath() to pull the user's temporary folder and just write files in there. Alternatively, you can use Environment.GetFolderPath(...) to grab one of the user special folders, preferably LocalApplicationData and create a subfolder in there for temporary storage. But I think the first approach of using Path.GetTempPath() is the best way to go.

This is advisable instead of writing to the application executable folder because the application executable folder is not guaranteed to be writable (it might be on a network volume, in a folder owned by another user, or a user might have copied it to Program Files even though it's not "installed", etc.). Even though this app package is portable, I would argue that expecting the app folder to be writable actually makes it less portable.

@beeradmoore
Copy link
Owner

Oh sorry, I misunderstood your usage of %temp%. I thought you meant download DLSS Swapper to temp and execute it from there 😅. I was confused how someone would run into this scenario but tested it anyway. My bad.

From memory the installer version does everything you said to do. Portable version does everything it can to not leave a trace on the PC (including putting anything in system temp) and keep everything relative to itself.

Moving downloads out of temp (folder relative to the exe in the StoredData folder) will only fix that one issue. Settings, the location of the fully downloaded DLSS dlss etc, will still try to go to system32. I have a writeup on what directories are used here (I think this is still up to date).

While I agree with you that the executable location may not be writable that would be a different problem all together. I think by definition of portable it means that if you put this on a USB drive, download DLSS dlls, then move that USB to another computer then all your DLSS dlls are there and ready to go.

@rennr
Copy link

rennr commented Mar 11, 2023

While I agree with you that the executable location may not be writable that would be a different problem all together. I think by definition of portable it means that if you put this on a USB drive, download DLSS dlls, then move that USB to another computer then all your DLSS dlls are there and ready to go.

OK that's fair and a reasonable use case.

Then I would suggest two things: (1) Try and see if the working folder is system32 on startup and, if so, set the working folder to the executable file path; (2) test to see if the working folder (or the executable file path, as the case may be) is writable and give an error if it is not and quit.

I assume that options 1 and 2 I set out above would only occur if this is the portable version, since the installed version would have different behaviour based on what you said (i.e., it uses local app data instead, and if that were not writable, that would certainly be an issue!)

@beeradmoore
Copy link
Owner

I didn't know I could change working directory while the application is running. That would fix things such as accidentally writing a random file out File.WriteAllText(... expecting it to be working directory. Ideally any File IO regarding pathing goes via the Storage class.

The fake temp directory I mentioned before is fetched from here. For portable app is is using storagePath which is actually set here.

In theory if I replace that Directory.GetCurrentDirectory() (and the only other places it's used in the entire project here with some other call that gets the executable file path then it should work as expected even if a user sets a custom working directory from a shortcut for some unknown reason.

@Oscillate18 , excellent news that you can still replicate. I need to duck out for a few hours but when I get back I'll work on a fix and get a 1.1 pre-release up for you to test when you are next available.

@beeradmoore
Copy link
Owner

So when working on the fix and this new test dialog on my other PC I was able to replicate the issue. For some reason the same steps I did on Win11 PC to launch from start menu search did not work, but doing the same on my Win10 PC did work 🤷‍♂️

The pre-release (v1.0.1.0) is available here.

When launching this app build you should get the following popup. If GetCurrentDirectory reports system32 then you would have had the same problem with this build. assemblyPath and baseDir ideally should both display the current path DLSS Swapper is executing from (one may have an additional trailing slash but that is fine).

image

@Oscillate18
Copy link
Author

So when working on the fix and this new test dialog on my other PC I was able to replicate the issue. For some reason the same steps I did on Win11 PC to launch from start menu search did not work, but doing the same on my Win10 PC did work 🤷‍♂️

The pre-release (v1.0.1.0) is available here.

When launching this app build you should get the following popup. If GetCurrentDirectory reports system32 then you would have had the same problem with this build. assemblyPath and baseDir ideally should both display the current path DLSS Swapper is executing from (one may have an additional trailing slash but that is fine).

image

Still the same issue unfortunately with the pre-release build.

Screenshot 2023-03-12 133630

@rennr
Copy link

rennr commented Mar 12, 2023

@beeradmoore Instead of doing this:

#elif PORTABLE
        static string storagePath => Path.Combine(Directory.GetCurrentDirectory(), "StoredData");
#else

Maybe something like this?

#elif PORTABLE
        static string storagePath => GetPortableStoragePathDir();
        static string cachedStoragePath = null;
#else

// ...

private static string GetPortableStoragePathDir() {

    // Don't do these checks if we've already done them once - just return cached data
    if (cachedStoragePath != null)
        return cachedStoragePath;
    
    var currentDirectory = Directory.GetCurrentDirectory();
    
    var systemFolder = Environment.GetFolderPath(Environment.SpecialFolder.System);
    
    if (currentDirectory.ToLower().Contains(systemFolder.ToLower())) {
        // Fall back to application folder if the current directory is the system folder
        currentDirectory = Assembly.GetExecutingAssembly().Location;
    }
    
    // Test to see if it's writable
    try {
        Directory.CreateDirectory(Path.Combine(currentDirectory, "StoredData"));
    } catch {
        // Probably failed to write - fall back to generic temp folder
        currentDirectory = Path.GetTempPath();
    }
    
    cachedStoragePath = Path.Combine(currentDirectory, "StoredData");
    return cachedStoragePath;
}

This solves a few problems:

(1) If the current folder is the system folder, use the executing assembly's folder
(2) If the folder isn't writable, fall back to a generic temp path. Frankly this is optional, but I think it would make sense to at least test for it. Maybe you can just have it display an error instead of falling back to the temp path.

Note that I didn't test this code and just wrote it from memory, so you may have to modify it for it to compile. But this is my general thoughts on a solution to this problem.

@beeradmoore
Copy link
Owner

@Oscillate18 if you see what you see in the screenshot it should behave normally. The first line saying system32 is just demonstrating it would have failed. The other two lines are saying where it will attempt to write files now.

Are you still getting issues downloading?

@beeradmoore
Copy link
Owner

@rennr , in theory that isn't needed. The fix is in, I think my instructions on how to test were just not great. The solution I did was this.

Your solution fixes problem 1, but with number 2 we don't want to fall back to writing all other things like settings to system temp, storing completed downloads in temp. At this point the application has failed and we shouldn't keep falling back and back and back.

But I 110% agree with what you said about we should throw a useful error. I created issue #128 to investigate this further as currently game sub-folders could be read-only and I am not sure exactly what the system does when this happens 👀. I'll test these other scenarios and try throw better messages where possible as if an error is thrown now it will just say something generic which is not useful to the end user.

@beeradmoore
Copy link
Owner

@rennr , @Oscillate18 , are either of you two able to confirm on your end that portable version of 1.0.1 pre-release works for this issue? Keeping in mind it should say system32 on the top line of that first prompt.

@Nypheena
Copy link

it seems I get this issue if I pin to start and launch from there, even with 1.0.1 it will launch in system if its pinned to start

@beeradmoore
Copy link
Owner

Hey @Nypheena , would you be able to upload a screenshot of the error you are getting when trying to download a DLSS dll with 1.0.1?

@Oscillate18
Copy link
Author

I'm able to download the files normally with the pre-release, from a search or pinned to the start menu. The first line did change from system32 when launched from being pinned to the start menu, that doesn't change when launched from a search. Not sure if it's an issue but thought I'd mention it.

Screenshot 2023-03-21 181925

@Nypheena
Copy link

Nypheena commented Apr 7, 2023

Hey @Nypheena , would you be able to upload a screenshot of the error you are getting when trying to download a DLSS dll with 1.0.1?

sorry didn't see this, it wasn't happening when downloading a DLSS I was getting the "GetCurrentDirectory:" as system32 and the DLSS I did have already downloaded wasn't showing but I have not been able to reproduce this issue since stopping to launch from the windows start menu and even if I do now it sticks to where it actually is

@beeradmoore
Copy link
Owner

This should be fixed in v1.0.2 which was just released. I am closing this issue for now. If it is still happening in this updated release comment below and I'll re-open this card.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants
  翻译: