Cocona icon indicating copy to clipboard operation
Cocona copied to clipboard

Fix a stack overflow in non-writable working directories

Open DavidL344 opened this issue 1 year ago • 3 comments

Fixes #128. By using the running application's directory as the base path, the package won't access possibly non-writable working directories.

https://github.com/mayuki/Cocona/assets/69001314/519c19c2-2dc9-4b3f-b6e5-ef3496a8eaea

DavidL344 avatar Feb 26 '24 15:02 DavidL344

Thanks for the PR!

It appears that the problem occurs in combination with the default registered configuration provider. I would like to investigate this in detail.

mayuki avatar Mar 05 '24 17:03 mayuki

It looks good to have ContentRoot in the same directory as the executable file. By the way, I couldn't reproduce it in my environment (Devcontainer), but could you tell me about your filesystem and kernel/OS?

mayuki avatar Mar 07 '24 13:03 mayuki

Sure thing! Here are the specs of the system I could reproduce the issue on:

  • OS: Ubuntu 22.04 LTS (x64)
  • Kernel: linux 6.5.0-21-generic
  • Filesystem: ext4

The issue also occurs on the Ubuntu Live CD as well - although the stack doesn't overflow there, the app still runs with a delay under previously mentioned directories (including the root path).

If you'd like to try reproducing the issue on the Ubuntu Live CD, this is a script I use to install .NET 8 under a live environment:

#!/bin/bash

# Resources:
# https://learn.microsoft.com/en-gb/dotnet/core/install/linux
# https://learn.microsoft.com/en-gb/dotnet/core/install/linux-ubuntu#register-the-microsoft-package-repository

sudo apt update
sudo apt install wget -y

# Get Ubuntu version
declare repo_version=$(if command -v lsb_release &> /dev/null; then lsb_release -r -s; else grep -oP '(?<=^VERSION_ID=).+' /etc/os-release | tr -d '"'; fi)

# Download Microsoft signing key and repository
wget https://packages.microsoft.com/config/ubuntu/$repo_version/packages-microsoft-prod.deb -O ~/Downloads/packages-microsoft-prod.deb

# Install Microsoft signing key and repository
sudo dpkg -i ~/Downloads/packages-microsoft-prod.deb

# Clean up
rm ~/Downloads/packages-microsoft-prod.deb

# Update packages
sudo apt update

# Install .NET 8.0 SDK
sudo apt install dotnet-sdk-8.0 -y

What I haven't accounted for is a scenario where the application could be installed and run from a read-only directory - that would mean the application's startup time would still be slow if the it were to be run from its installed path. Maybe a check for read-write permissions in the working directory (and changing the ContentRoot to a read-writable one if the current one is not) could solve the problem!

DavidL344 avatar Mar 08 '24 20:03 DavidL344

There's one thing I've completely missed - I wasn't really aware what the FileSystemWatcher does, but I've noticed that the reason why it's slower in certain directories and faster in others is not whether or not they're read-only, but rather the amount of content they have in them.

According to #132, the FileSystemWatcher in this case recursively lists all files in the current directory, which in this case, is the running application's working directory. By changing the working directory to where the application is running from, there's a higher chance of the directory not containing as many files, which speeds up the application's startup time, since the FileSystemWatcher lists less files.

So, what my fix essentially does, is it always makes the FileSystemWatcher recursively read files from the directory the application is stored in, which makes the startup time the same from everywhere. And since that directory doesn't have nearly as many files as other ones the app might be running from (like the home directory, or even worse, the root directory, which makes the app list the entire filesystem's contents before running), I believe this could be a decent fix for now, at least before any other PR addressing this issue in a better way is created.

Are there any changes I should make before the PR could be considered ready? Thank you!

DavidL344 avatar Apr 04 '24 16:04 DavidL344

Apologies for the delay, but I have merged it. Thanks!

mayuki avatar Jun 23 '24 13:06 mayuki

@mayuki would it be possible to release a new nuget version with this fix?

I'd like to stop using my solution from #132 as that's a lot of unnecessary boilerplate.

GGG-KILLER avatar Sep 03 '24 14:09 GGG-KILLER