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

possible rm -rf / during screen installation #488

Closed
Matszwe02 opened this issue Jul 7, 2024 · 4 comments
Closed

possible rm -rf / during screen installation #488

Matszwe02 opened this issue Jul 7, 2024 · 4 comments
Labels
klipperscreen Something that is klipperscreen related ❓ question Further information is requested

Comments

@Matszwe02
Copy link

Linux Distribution

rpiOS legacy 32bit on rpi3b

What happened

rm -rf /

What did you expect to happen

idk what should happen, raised an exception, install successful, either way, not rm -rf /

How to reproduce

Additional information

I don't have any logs, because, yeah...

@dw-0
Copy link
Owner

dw-0 commented Jul 8, 2024

When choosing to install KlipperScreen with KIAUH, it's actually KlipperScreens own install script that gets executed by KIAUH.
I had a look at the script and the only line containing a rm -rf was this: https://github.com/KlipperScreen/KlipperScreen/blob/master/scripts/KlipperScreen-install.sh#L120
The referenced variable {KSENV} is defined here: https://github.com/KlipperScreen/KlipperScreen/blob/master/scripts/KlipperScreen-install.sh#L5

I am not sure if there is any constellation where the value of KSENV may be / at line 120 in the script.
@alfrix Any idea if and how this could have happened?

@dw-0 dw-0 added klipperscreen Something that is klipperscreen related ❓ question Further information is requested labels Jul 8, 2024
@alfrix
Copy link

alfrix commented Jul 8, 2024

When choosing to install KlipperScreen with KIAUH, it's actually KlipperScreens own install script that gets executed by KIAUH.

Before KIAUH runs KlipperScreen's installation script, KIAUH removes any existing KlipperScreen directory
[[ -d ${KLIPPERSCREEN_DIR} ]] && rm -rf "${KLIPPERSCREEN_DIR}"

[[ -d ${KLIPPERSCREEN_DIR} ]] && rm -rf "${KLIPPERSCREEN_DIR}"

leading into a first possible case of this unintended behavior if KLIPPERSCREEN_DIR is evaluated to "/" for some reason

then KS install script is executed
the install script will set:

KSENV="${KLIPPERSCREEN_VENV:-${HOME}/.KlipperScreen-env}"
...
    if [ -d $KSENV ]; then
        echo_text "Removing old virtual environment"
        rm -rf ${KSENV}
    fi

which means if KLIPPERSCREEN_VENV is set use that, and if that is evaluated into "/" you would have the same possible issue again

the else uses /home/username/.KlipperScreen-env, even if HOME is evaluated into nothing the tail /.KlipperScreen-env should ensure that nothing bad will happen with the default

It's worth noting that nor KLIPPERSCREEN_DIR or KLIPPERSCREEN_VENV are set by KlipperScreen or it's standalone installer, those are external variables for other installers

This leads me to the conclusion that every rm -rf should be double checked against "/" just in case, even if i find this scenario very unlikely

@dw-0
Copy link
Owner

dw-0 commented Jul 8, 2024

Before KIAUH runs KlipperScreen's installation script, KIAUH removes any existing KlipperScreen directory [[ -d ${KLIPPERSCREEN_DIR} ]] && rm -rf "${KLIPPERSCREEN_DIR}"

[[ -d ${KLIPPERSCREEN_DIR} ]] && rm -rf "${KLIPPERSCREEN_DIR}"

leading into a first possible case of this unintended behavior if KLIPPERSCREEN_DIR is evaluated to "/" for some reason

Ah yes you are correct, there is a rm -rf before executing the KlipperScreen script. KLIPPERSCREEN_DIR is defined here: https://github.com/dw-0/kiauh/blob/master/scripts/globals.sh#L51

From my perspective that cannot be "/" in any case, nor can the KSENV from your script be "/".

No idea what happened there...

@Matszwe02
Copy link
Author

I suspect that something might be off because I didn't have klipper installed, as the second time, that I installed ks after klipper, it worked.

I saw that it broke after or during installation of requirements for venv

alfrix added a commit to KlipperScreen/KlipperScreen that referenced this issue Jul 14, 2024
@dw-0 dw-0 closed this as completed Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
klipperscreen Something that is klipperscreen related ❓ question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants