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

Moved ~/.local/share/steam. Ran steam. It deleted everything on system owned by user. #3671

Closed
keyvin opened this issue Jan 14, 2015 · 265 comments
Closed
Labels

Comments

@keyvin
Copy link

@keyvin keyvin commented Jan 14, 2015

Edit: Please stop posting stupid image memes or unhelpful messages. This interferes with Valve's ability to sift through the noise and see if anyone can figure out what triggers it.

This may not be a common problem because I change all sorts of configuration about my system. The script in question does something in a really, really stupid way, but it probably doesn't trigger the fail scenario for every system because...

Original Bug:
I am not sure what happened. I moved the folder in the title to a drive mounted under /media/user/BLAH and symlinked /home/user/.local/steam to the new location.

I launched steam. It did not launch, it offered to let me browse, and still could not find it when I pointed to the new location. Steam crashed. I restarted it.

It re-installed itself and everything looked great. Until I looked and saw that steam had apparently deleted everything owned by my user recursively from the root directory. Including my 3tb external drive I back everything up to that was mounted under /media.

Everything important, for the most part, was in the cloud. It is a huge hassle, but it is not a disaster. If there is the chance that moving your steam folder can result in recursively deleting everything in the directory tree you should probably just throw up an error instead of trying to point to other stuff. Or you know, allow the user to pick an install directory initially like on windows.

My system is ubuntu 14.04, and the drive I moved it to was ntfs if its worth anything.

@doofy
Copy link

@doofy doofy commented Jan 15, 2015

I am impressed how calm you stay about this. This is terrible. I just lost my home directory. All i did was start steam.sh with STEAM_DEBUG=1. I will investigate this and report back.

edit1: I suspect steam.sh got some bugs(does not check own variables) and when it tried to do scary things it crapped himself.
Line 468: rm -rf "$STEAMROOT/"*

edit2: It gets better. Seems on windows Steam is overeager too! https://support.steampowered.com/kb_article.php?ref=9609-OBMP-2526 (The warning part is interesting. Because everybody reads this before uninstalling...)

@Tele42
Copy link

@Tele42 Tele42 commented Jan 15, 2015

I agree, that line minimally requires an exists and not null check for $STEAMROOT

@keyvin
Copy link
Author

@keyvin keyvin commented Jan 15, 2015

#scary!

As an ex programmer, that really makes me chuckle. Can I at least get an apology from whoever committed that comment without adding a fix?

@onodera-punpun
Copy link

@onodera-punpun onodera-punpun commented Jan 15, 2015

This also happened to me a few weeks ago, my entire home was deleted by the steam.sh script.

@pythoneer
Copy link

@pythoneer pythoneer commented Jan 15, 2015

introduced here Sep 10, 2013 https://github.com/indrora/steam_latest/commit/21cc14158c171f5912b04b83abf41205eb804b31 line 359

rm -rf "$STEAMROOT/"* could be evaluated as rm -rf "/"* if $STEAMROOT is empty

but what exactly caused this? i've symlinked ~/.local/share/steam to, so i am a bit afraid to start steam :/

@TcM1911
Copy link

@TcM1911 TcM1911 commented Jan 15, 2015

pythoneer,

I believe the issue starts on line 19:

# figure out the absolute path to the script being run a bit
# non-obvious, the ${0%/*} pulls the path out of $0, cd's into the
# specified directory, then uses $PWD to figure out where that
# directory lives - and all this in a subshell, so we don't affect
# $PWD

STEAMROOT="$(cd "${0%/*}" && echo $PWD)"
STEAMDATA="$STEAMROOT"

This probably returns as empty which mean: rm -rf "$STEAMROOT/"* is the same ass rm -rf "/"*.

@pythoneer
Copy link

@pythoneer pythoneer commented Jan 15, 2015

TcM1911,

that's my guess, too.

@ghost
Copy link

@ghost ghost commented Jan 15, 2015

@keyvin @d00fy :
Line proceeded by # Scary! comment is in reset_steam() function, which is executed if and only if steam.sh is invoked with --reset as first argument.

Did any of you deliberately invoked that script with that option? If yes, why did you do it? What were you trying to achieve?

Removing user data is obviously wrong, no doubt about it. But if this happens only when user requests certain action, scope of that issue is somewhat limited.

@jthill
Copy link

@jthill jthill commented Jan 15, 2015

Yeah, they kinda need a readlink in there.

STEAMROOT=$(readlink -nf "${0%/*}")
@jthill
Copy link

@jthill jthill commented Jan 15, 2015

@minio Not "only if". reset_steam is also invoked by removing your .steam directory, since that sets INITIAL_LAUNCH

@soren121
Copy link

@soren121 soren121 commented Jan 15, 2015

@minio A script accidentally running rm -rf /* is unacceptable in any scenario.

@gtmanfred
Copy link

@gtmanfred gtmanfred commented Jan 15, 2015

it is like bumblebee all over again!
MrMEEE/bumblebee-Old-and-abbandoned#123

@Plaque-fcc
Copy link

@Plaque-fcc Plaque-fcc commented Jan 15, 2015

I encountered Steam behaviour like with «--reset» for several times:
when I added «--reset» AND when I didn't. So — not «only if», can
confirm this, even having not deleted ~/.steam/ dir, too.

@prometheanfire
Copy link

@prometheanfire prometheanfire commented Jan 15, 2015

wonder what the code path is to hit that rm without --reset

@indrora
Copy link

@indrora indrora commented Jan 15, 2015

Can confirm; I have Steam bounded in an SELinux context ("Steam") and SELinux spits out:

Context violation: process /home/indrora/.local/share/Steam/ubuntu12_32/steam is only allowed in context steam_context, attempted to remove /boot/efi/grub/efistub

Ooops. I'll write a patch and PR it 🍺

@johnv-valve johnv-valve self-assigned this Jan 15, 2015
@johnv-valve
Copy link
Contributor

@johnv-valve johnv-valve commented Jan 15, 2015

Does anybody have reliable repro steps for this? I can easily add the checks for STEAMROOT being empty, but I would also like to fix the root cause if possible.

@rcxdude
Copy link

@rcxdude rcxdude commented Jan 15, 2015

It will definitely fail if you run steam.sh as bash steam.sh. I don't know if that's the cause in this case. In terms of root cause, I would say you should use set -e, set -u, and similar options in order to make the script less likely to silently ignore errors.

@ayust
Copy link

@ayust ayust commented Jan 15, 2015

Using ${STEAMROOT:?} instead of $STEAMROOT would have helped, too.

(For those not familiar, ${FOO:?} is identical to $FOO except that it errors out automatically if $FOO is empty or unset.)

@ju2wheels
Copy link

@ju2wheels ju2wheels commented Jan 16, 2015

Which is the same thing that would have happened had the unnecessary '/*' not been there anyway, it would have errored out. Its not necessary because the rm was set to recursive already...

if [ ! -z "${STEAMROOT}" ]; then
   rm -rf "${STEAMROOT}"
fi
@rcxdude
Copy link

@rcxdude rcxdude commented Jan 16, 2015

Here is a patch which enables set -e, set -u, and a few others, and then fixes up all the places where undefined variables are expected to be (as far as I can tell): https://gist.github.com/rcxdude/1f6257e0a965147a462c

@mablae
Copy link

@mablae mablae commented Jan 16, 2015

@rcxdude Come on. We are on github here. Do that in a PR please! Do not post whole patch files into issues... 👎

@dannyfallon
Copy link

@dannyfallon dannyfallon commented Jan 16, 2015

@mablae There is no code on this repo, there is nothing to send in a PR against. A gist wouldn't have gone astray though 😄

@s11k
Copy link

@s11k s11k commented Jan 16, 2015

@rcxdude: Please link to a Gist.

@sdt16
Copy link

@sdt16 sdt16 commented Jan 16, 2015

@mablae How about instead of being a jerk, you link @rcxdude to some documentation.

@Tele42
Copy link

@Tele42 Tele42 commented Jan 16, 2015

@johnv-valve regardless of tracking down the cause of this, this rm line must be protected from future accidental gremlins due to the severity of the fail scenario.

@mablae
Copy link

@mablae mablae commented Jan 16, 2015

@dannyfallon Oh, sorry then...
@sdt16 I am sorry. Thought the code was on this repo ... Didn't wanted to be a "jerk" :)

@Wapaca
Copy link

@Wapaca Wapaca commented Jan 16, 2015

Does it happen only if you move ~/.local/share/steam ? #scary! :s

@joshenders
Copy link

@joshenders joshenders commented Jan 16, 2015

The idiomatic way to do this in Bash is to use default variables as in ,"${var:-/sane/path}" or "${var:?}" as was already mentioned. While using set -u or similar could have prevented this mistake, it's lazy and considered bad style.

@craig-sanders
Copy link

@craig-sanders craig-sanders commented Oct 15, 2017

you may think you were making some significant point, but really you were just wishing that steam was a real package manager. It isn't, and no amount of bitching about it is ever going to get Valve to devote the time and resources to make it one. There's little or no benefit to them in doing so - steam mostly functions well enough at what it is, a games library manager.

as for your "wild guessing" comment and request for an explanation of the limit - we're talking here about the steam.sh shell script, which is a wrapper around the steam application, doing environment setup and other things (like helpfully deleting all user files on certain error conditions). Rewriting the steam app so that it acts like a package manager should is way beyond the scope of fixing the obvious errors in a shell script. I would have thought that was obvious.

In case you hadn't noticed, Valve don't even acknowledge the fact that their crappy steam.sh script is still broken and puts user files at risk - there's some small hope that they might eventually do that and adopt some of the ideas and suggestions in this thread (there's some evidence they've already done that for some things), but there's no way they'll ever re-write the steam app to incorporate the basic features of a package manager.

@hasufell
Copy link

@hasufell hasufell commented Oct 15, 2017

you may think you were making some significant point, but really you were just wishing that steam was a real package manager

Again, no.

as for your "wild guessing" comment and request for an explanation of the limit - we're talking here about the steam.sh shell script

No, we are talking about steam as a whole.

Rewriting the steam app so that it acts like a package manager should is way beyond the scope of fixing the obvious errors in a shell script. I would have thought that was obvious.

Again, no. This is not about package managers. You seem to have no understanding of how the proposed technology works.

This is not about fixing half-assed shell scripts, which are already by concept wrong. This is about correctness, which is crucial when you deal with user files. Recursive deletion is never correct from an automated cleanup POV. Recursive deletion is something a user may trigger, based on his needs.

but there's no way they'll ever re-write the steam app

First, there is no rewriting of the "steam app" involved. Second, that's guessing again.

@aaronfranke
Copy link

@aaronfranke aaronfranke commented Oct 15, 2017

@craig-sanders

https://www.freedesktop.org/wiki/Software/systemd/TheCaseForTheUsrMerge/

Solaris started it, Fedora was one of the first Linux distros to do it.

@mittwinter
Copy link

@mittwinter mittwinter commented Oct 15, 2017

@aaronfranke really? name these "many distros". in fact, name one. bash has been in /bin on linux forever because that's where system-native shells belong. third-party shells tend to go in /usr/local/bin. or in some bizarre location like /usr/opt/local/where/the/fuck/is/it on solaris.

@craig-sanders: Sorry to barge in, but maybe you have to update your knowledge on this one:

And probably others already, as this merge of /bin to /usr/bin was advocated by systemd, so probably many other distros that switched to systemd will eventually follow.

@ghost
Copy link

@ghost ghost commented Oct 15, 2017

No big deal since the merge to /usr/bin is done slowly and places symlinks for maintaining backwards compatibility.

2017-10-15-194019_499x65_scrot

So using /bin/bash is safe on “traditional” systems as well as on nowadays systems.

@ju2wheels
Copy link

@ju2wheels ju2wheels commented Oct 15, 2017

Guys, we are bike shedding solving a problem which has already had a suitable solution presented and alternatives. Discussing it ad nauseam isnt going to make Valve come out and fix it if they have no interest, we have suitably voiced opinion here. Lets post a link to some other forum (Reddit, Google groups, or elsewhere) and take the sidebar discussion there.

@mittwinter
Copy link

@mittwinter mittwinter commented Oct 15, 2017

No big deal since the merge to /usr/bin is done slowly and places symlinks for maintaining backwards compatibility.
So using /bin/bash is safe on “traditional” systems as well as on nowadays systems.

This may be true so far, but every backwards compatibility will eventually be going away.
And this:

#!/usr/bin/env bash. bash is always in /bin on a linux system, also on OS X. and using env to find a script interpreter is brain-damaged.

is certainly outdated advice for any software maintained currently or in the future. Distributions will have to do enough patching for legacy software once such backwards compatibility is faded out.

@polkovnikov-ph
Copy link

@polkovnikov-ph polkovnikov-ph commented Oct 16, 2017

@ju2wheels But a Half Life 3 talk didn't even have a chance to start!

@user15177
Copy link

@user15177 user15177 commented Nov 28, 2017

Has this problem been fixed? I really want to play the metro series, but this scares the crap out of me.

@ghost
Copy link

@ghost ghost commented Nov 28, 2017

@user15177 Set up a chroot for Steam and run it from there so it can do no harm to your actual system.

@alphapapa
Copy link

@alphapapa alphapapa commented Jan 1, 2018

@craig-sanders Since you posted some cleaned-up code from the script and some good Bash scripting advice, a note: Always use the [[ ]] test syntax in Bash-specific scripts. It's safer and cleaner in all cases.

ethomson added a commit to ethomson/azure-pipelines-agent that referenced this issue Feb 19, 2019
When expanding variables to pass to `rm`, make sure that the path
variable is set and fail if it is not.  This prevents an `rm` from
accidentally expanding to `rm ""/*` when the variable is unset:
ValveSoftware/steam-for-linux#3671

Using `${FOO:?}` syntax will fail when `FOO` is unset.
TingluoHuang added a commit to microsoft/azure-pipelines-agent that referenced this issue Feb 19, 2019
* dev.sh: use dash instead of slash for msbuild

Instead of switching between windows and non-windows to determine how to
handle slashes for msbuild, use dashes instead of slashes to simplify
the calling.

* dev.sh: stop on errors

Stop on errors, instead of continuing.  This prevents us from failing to
move through the directory space with `cd` / `pushd` / `popd` but still
running commands.  This is particularly dangerous when running commands
like `rm`.

* dev.sh: quote all filepaths

Since directories may have a space in them, quote them to treat them as
a single entity instead of wordsplitting on a space.

Otherwise, if `FOO="a b c"` then `rm -rf $FOO` will remove files or
folders named `a`, b`, and `c` instead of removing the single entity
named `a b c`.

* dev.sh: remove files carefully

When expanding variables to pass to `rm`, make sure that the path
variable is set and fail if it is not.  This prevents an `rm` from
accidentally expanding to `rm ""/*` when the variable is unset:
ValveSoftware/steam-for-linux#3671

Using `${FOO:?}` syntax will fail when `FOO` is unset.

* dev.sh: quote the `$` in `$LastExitCode`

`$LastExitCode` is not a bash variable; to pass that string along to
PowerShell, it needs to be quoted.

* dev.sh: use $(cmd) syntax instead of backticks

The $(cmd) execution syntax is preferred over the legacy backtick
syntax.

* dev.sh: quote variables

* externals.sh: quote variables to cope with spaces

Quote the variables for the directories so that we can properly work
with directories with spaces in their names.
@aviwad
Copy link

@aviwad aviwad commented Mar 17, 2019

Is this fixed? xD

@aviwad
Copy link

@aviwad aviwad commented Mar 17, 2019

whoa what the heck does the unassigned message mean

@aaronfranke
Copy link

@aaronfranke aaronfranke commented Mar 17, 2019

@aviwad It's just something GitHub does automatically when the assigned person doesn't actually do anything about the issue and someone comments on it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.