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

Ensure that ROOTFS_DIR variable is set with non-null value. #1440

Closed
wants to merge 1 commit into from

Conversation

jstourac
Copy link

@jstourac jstourac commented Aug 8, 2017

  • In case ROOTFS_DIR would, by an accident, was set empty or not to set at all, then system
    level configuration files would be overwritten then.

Actually there are quite a lot more occurrences like this. I guess I could look at it. Let me know if you are ok to change all of those or not.

- In case ROOTFS_DIR would by an accident stayed empty, then system
  level configuration files would be overwritten then.
@schlomo
Copy link
Member

schlomo commented Aug 8, 2017

@jstourac In general this is a great idea to make ReaR more robust. Can you please let us know why you started from this particular script? Is there some context for this choice?

In #700 we discussed making ReaR work under set -eu and came to the conclusion that this is nearly impossible at the moment.

Instead of fixing this one script I can imagine to add a more generic script somewhere else that will ensure that certain variables are set, like an assertion:

: ${ROOTFS_DIR:?must be set}

That way all scripts would be protected, not just this one.

@jstourac
Copy link
Author

Hi @schlomo,

reason I started with this one script is because I suspected that my configuration of sshd on my RHEL6 testing machine was modified by execution of {{/usr/sbin/rear checklayout || /usr/sbin/rear mkrescue}}, used ReaR was in version of 1.17.2. Although, I was not able to reproduce and neither I could not find out how that would be possible after I briefly looked in the source code.

Anyway, I think that such check is useful to prevent potential modifications of system files, that is why I proposed this MR to start a discussion.

@schlomo
Copy link
Member

schlomo commented Aug 14, 2017

I see. What do you think about adding assertions somewhere higher up or earlier in the script execution flow?

@jsmeix
Copy link
Member

jsmeix commented Aug 21, 2017

I think ensure that certain variables are set is something for
#1251

@gdha
Copy link
Member

gdha commented Mar 12, 2018

As nobody is complaining about this in the past I'm pro closing this PR without further implementation.

@gdha gdha closed this Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants