Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10541017:
Add update-linux-sandbox.sh to build/ (Closed)
|
Created:
8 years, 6 months ago by jln (very slow on Chromium) Modified:
8 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd update-linux-sandbox.sh to build/
This script is a useful helper to make sure the setuid sandbox is
up-to-date after building Chrome.
BUG=
TEST=
NOTRY=true
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140816
Patch Set 1 #
Total comments: 42
Patch Set 2 : Address review comments #
Total comments: 4
Patch Set 3 : mu #
Total comments: 4
Patch Set 4 : simplify if statement #
Total comments: 1
Patch Set 5 : Properly enclose variables inside strings #Patch Set 6 : Address comments by Markus and Lei #Patch Set 7 : Warn before using sudo as per Markus request #
Total comments: 4
Patch Set 8 : Check for NFS in code + mu #Messages
Total messages: 18 (0 generated)
Hi Lei, I would like to make this script available somewhere and to reference it in https://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment Is this a good place?
https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:4: CHROME_OUT_DIR="${CHROME_SRC_DIR}/out/${BUILDTYPE}" Have you considered putting the dev sandbox at say, /usr/local/sbin/chrome-devel-sandbox? Developers may clobber out/ from time to time. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:24: sudo -- cp $chrome_sandbox_build_path $chrome_sandbox_installed_path && nit: put variable in double quotes here and below. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:46: if [ ! x$current_api = x$installed_api ] nit: You can do: if [ "$foo" != "$bar" ] Same below. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:49: echo "installing it now." nit: this fits on the previous line.
https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:1: #!/bin/sh add -e add standard Chromium copyright header https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:2: BUILDTYPE=${BUILDTYPE:-"Debug"} insert a blank line before line 2 https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:10: then please use this syntax: if [ ... ]; then
https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:10: then On 2012/06/05 22:11:29, cmp wrote: > please use this syntax: > > if [ ... ]; > then Did you mean "if [ ... ]; then" ?
https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:10: then I'm retracting this comment since the scripts in src/ and other places are just not internally consistent on this point. At least Julien's script is consistent, so that's enough for me IMO here.
PTAL https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:1: #!/bin/sh On 2012/06/05 22:11:29, cmp wrote: > add -e > > add standard Chromium copyright header I specifically didn't do it because I wanted to give more useful error messages. For instance see lines 37 to 41. WDYT ? https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:2: BUILDTYPE=${BUILDTYPE:-"Debug"} On 2012/06/05 22:11:29, cmp wrote: > insert a blank line before line 2 Done. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:4: CHROME_OUT_DIR="${CHROME_SRC_DIR}/out/${BUILDTYPE}" On 2012/06/05 22:07:45, Lei Zhang wrote: > Have you considered putting the dev sandbox at say, > /usr/local/sbin/chrome-devel-sandbox? Developers may clobber out/ from time to > time. Done. I was wondering if having a global one could be annoying if people play with multiple repositories at the same time, but I think it's likely that you suggested change is an improvement for the vast majority. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:24: sudo -- cp $chrome_sandbox_build_path $chrome_sandbox_installed_path && On 2012/06/05 22:07:45, Lei Zhang wrote: > nit: put variable in double quotes here and below. Done. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:46: if [ ! x$current_api = x$installed_api ] On 2012/06/05 22:07:45, Lei Zhang wrote: > nit: You can do: if [ "$foo" != "$bar" ] > Same below. Done. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:49: echo "installing it now." On 2012/06/05 22:07:45, Lei Zhang wrote: > nit: this fits on the previous line. Done.
lgtm with thestig's lgtm no prob about -e https://chromiumcodereview.appspot.com/10541017/diff/1002/build/update-linux-... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1002/build/update-linux-... build/update-linux-sandbox.sh:6: BUILDTYPE=${BUILDTYPE:-"Debug"} nit: really the empty line should happen before BUILDTYPE, point taken that i said "before line 2," though. can you make that change here? (remove empty line 2, insert an empty line before "BUILDTYPE") https://chromiumcodereview.appspot.com/10541017/diff/1002/build/update-linux-... build/update-linux-sandbox.sh:49: if [ ! $? = 0 ]; then i believe lines 48-49 can be simplified as: if ! installsandbox; then
https://chromiumcodereview.appspot.com/10541017/diff/1002/build/update-linux-... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1002/build/update-linux-... build/update-linux-sandbox.sh:6: BUILDTYPE=${BUILDTYPE:-"Debug"} On 2012/06/05 22:54:20, cmp wrote: > nit: really the empty line should happen before BUILDTYPE, point taken that i > said "before line 2," though. can you make that change here? (remove empty > line 2, insert an empty line before "BUILDTYPE") I think there was a race condition, I had already uploaded a minor update to address that. It looks like scripts usually have a blank like both before and after the copyright. That's what the new version does. Let me know if that's not ok. https://chromiumcodereview.appspot.com/10541017/diff/1002/build/update-linux-... build/update-linux-sandbox.sh:49: if [ ! $? = 0 ]; then On 2012/06/05 22:54:20, cmp wrote: > i believe lines 48-49 can be simplified as: > if ! installsandbox; then Done, thanks.
I'll take a look at patch set 4 in a second. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:4: CHROME_OUT_DIR="${CHROME_SRC_DIR}/out/${BUILDTYPE}" On 2012/06/05 22:43:08, Julien Tinnes wrote: > On 2012/06/05 22:07:45, Lei Zhang wrote: > > Have you considered putting the dev sandbox at say, > > /usr/local/sbin/chrome-devel-sandbox? Developers may clobber out/ from time to > > time. > > Done. > > I was wondering if having a global one could be annoying if people play with > multiple repositories at the same time, but I think it's likely that you > suggested change is an improvement for the vast majority. I think most people would want to run this once and forget about it. Even with multiple repositories, they will likely be on the same sandbox API version. https://chromiumcodereview.appspot.com/10541017/diff/5002/build/update-linux-... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/5002/build/update-linux-... build/update-linux-sandbox.sh:9: CHROME_OUT_DIR="${CHROME_SRC_DIR}/out/${BUILDTYPE}" There's some developers who have custom gyp settings to put this elsewhere. Can we make this CHROME_OUT_DIR="${CHROME_OUT_DIR:$-{CHROME_SRC_DIR}/out/${BUILDTYPE}}" and adjust the error message on line 15 accordingly? https://chromiumcodereview.appspot.com/10541017/diff/5002/build/update-linux-... build/update-linux-sandbox.sh:11: # Make sure the path below is not on NFS! You can check this with: stat -f -c %t /usr/local/sbin I think nfs is type "6969". https://chromiumcodereview.appspot.com/10541017/diff/5002/build/update-linux-... build/update-linux-sandbox.sh:47: if [ x$current_api != x$installed_api ]; then nit: if you use quotes here and on line 56, then you don't need the x's. https://chromiumcodereview.appspot.com/10541017/diff/5002/build/update-linux-... build/update-linux-sandbox.sh:48: echo -n "Your installed setuid sandbox is too old, installing it now." nit: no need for the " -n"
https://chromiumcodereview.appspot.com/10541017/diff/2002/build/update-linux-... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/2002/build/update-linux-... build/update-linux-sandbox.sh:44: current_api=$($chrome_sandbox_build_path --get-api) Please double quote $chrome_sandbox_build_path here and $chrome_sandbox_installed_path below. Otherwise they won't work if there's spaces in the path.
This is probably more detailed then what you would normally get from me. But since you mentioned that you don't know much about shell scripting, I was very detailed and added a lot of comments about good practices for shell programming. In fact, I didn't spend any effort in checking whether the overall logic of the script is correct. I suspect, the other reviewers will do that. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:1: #!/bin/sh /bin/sh is OK and it results in slightly faster execution and is slightly more portable. These are all desirable features when writing code that needs to run on lots of different systems. In practice, I recommend you use /bin/bash, though. There are a whole bunch of bash-specific extensions that make your script much easier to maintain. Furthermore, you might want to consider enforcing strict error handling. This requires more care in your script, but it is more likely to stop the script early, if something went wrong. Rather than letting it stomp all over your file system: #/bin/bash -e If you do this, you might want to use trap handlers to let the user know, if the script exited unexpectedly. At the top of your script, add: trap 'trap "" EXIT ERR INT HUP TERM QUIT; { tput bel 2>/dev/null || :; printf "\n\nExiting unexpectedly!\n"; } >&2; exit 1' EXIT ERR INT HUP TERM QUIT And at the bottom of you script you do: trap '' EXIT ERR INT HUP TERM QUIT If you need to do some kind of clean up, whenever your script fails, that would also be something you can do from the trap handler. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:2: BUILDTYPE=${BUILDTYPE:-"Debug"} You don't really need the quotes around the string, although it doesn't do any harm. But you absolutely should have quotes around any expansion. So, either one of the following is correct: BUILDTYPE="${BUILDTYPE:-"Debug"}" or BUILDTYPE="${BUILDTYPE:-Debug}" The latter seems the more common way to do things. But I believe the shell treats both expressions exactly the same. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:3: CHROME_SRC_DIR=${CHROME_SRC_DIR:-$(dirname $(readlink -fn $0))/..} There are a whole bunch of variable expansions here, that really should be quoted. Otherwise, you can run into nasty surprises. The most common problem would be with file and directory names that include white space. CHROME_SRC_DIR="${CHROME_SRC_DIR:-$(dirname -- "$(readlink -f -- "${0}")")/..)" Also note that we don't really need "-n". The shell strips trailing newlines for us. This is still not quite what we want though, since it is somewhat ugly that there now are "/.." characters in the path. You can either do another call to readlink: CHROME_SRC_DIR="${CHROME_SRC_DIR:-$(readlink -f -- "$(dirname -- "$(readlink -f -- "${0}")")/..")}" Or you can admit that this is getting to long and write a version that uses shell built-ins only. It's a little more verbose, but also more readable and faster: [ -z "${CHROME_SRC_DIR}" ] && { CHROME_SRC_DIR="$(readlink -f -- "${0}")" CHROME_SRC_DIR="${CHROME_SRC_DIR%/*/*}" } I would probably pick this solution. But the earlier option is valid, too. In fact, the earlier solution is possibly more correct, if we expect to have crazy symbolic links (as "readlink", by definition, expands links). On the other hand, it depending on how the links are set, it could also do the wrong thing. Without more knowledge about what the user intended to do, this probably can't be decided. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:5: chrome_sandbox_build_path="${CHROME_OUT_DIR}/chrome_sandbox" Any reason for you to use lower-case variables here? Some people use upper-case for variables that they export to child processes, and they use lower-case for variables that do not get exported. But this is not very commonly done. A more common pattern is to use upper-case for global variables, and lower-case for local variables. This makes sense since it is not always easy to see which variables are local. But since you don't use any functions, I don't think this distinction would apply here. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:6: # Make sure the path below is not on NFS! This comment doesn't seem to reflect what the code is doing. Do you really want to check for NFS? Or do you want to check for "nosuid"? Or for both? You could for instance do: if grep "^$(df "${xxx}"|sed -n '${s/ .*//;p}') " /proc/mounts | egrep 'nfs|nosuid' >&/dev/null; then echo "Error! ..." >&2 exit 1 fi https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:9: if [ ! -d "$CHROME_OUT_DIR" ] You don't technically need curly braces around this environment variable. But it is generally good practice. So, please write: "${CHROME_OUT_DIR}" Also, I would usually put the "then" on the same line as the "if". But I don't think we have hard rules either way: if [ ! -d "${CHROME_OUT_DIR}" ]; then https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:11: echo -n "$CHROME_OUT_DIR does not exist. Use \"BUILDTYPE=Release ${0}\" " Again, missing curly braces. And we are missing any command line arguments. It would be nice if we printed those, wouldn't it. Furthermore, "echo -n" is a little fragile. Slightly safer to use "printf": printf '%s' "\"${CHROME_OUT_DIR}\" doesn't exist. Use \"BUILDTYPE=Release ${0}${*:+ ${*}}\". " https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:12: echo "If you are building in Release mode" This is likely going to result in a line that is longer than 80 characters. So, you might want to reconsider omitting the "\n" character. Of course, if you want to be really neat, you can pipe the output through "fold -s". Use the following to compute the number of columns: col="$(stty -a 2>/dev/null|sed 's/.*columns *\([0-9]*\).*/\1/;t1;d;:1;q')" col="${col:-80}" If you do decide to go this route, and I am not necessarily saying you should, then the line-wrapping code should probably go into a helper function. In fact, you might occasionally encounter machines that don't have a copy of "fold", so you should check for that too. And you might want to redirect all error messages to stderr: wrap() { local col col="$(stty -a 2>/dev/null|sed 's/.*columns *\([0-9]*\).*/\1/;t1;d;:1;q')" col="${col:-80}" local msg="$(cat)" printf '%s\n' "${msg}" | fold -s -w "${col}" >&2 2>/dev/null || printf '%s\n' "${msg}" >&2 } https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:16: if [ ! -f "$chrome_sandbox_build_path" ] See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:18: echo -n "Could not find $chrome_sandbox_build_path, " See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:23: installsandbox() { How come, this is a function. But nothing else in your code seems to be a function. Also, this is a somewhat unusual location to define the function. Casual readers might think it is executed right here. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:24: sudo -- cp $chrome_sandbox_build_path $chrome_sandbox_installed_path && We really need quotes. Bad things will otherwise happen. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:30: if [ ! -f "$chrome_sandbox_installed_path" ] See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:32: echo -n "Could not find $chrome_sandbox_installed_path, " See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:37: if [ ! -f "$chrome_sandbox_installed_path" ] See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:39: echo "Failed to install $chrome_sandbox_installed_path" See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:43: current_api=$($chrome_sandbox_build_path --get-api) current_api="$("${chrome_sandbox_build_path}" --get-api 2>/dev/null)" https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:44: installed_api=$($chrome_sandbox_installed_path --get-api) See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:46: if [ ! x$current_api = x$installed_api ] I think, "bash" is actually smart enough to not need the "x". But you are correct and it is good practice to do so anyway. Nonetheless, we really want proper quoting: if [ "x${current_api}" != "x${installed_api}" ]; then https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:51: if [ ! $? = 0 ] If comparing numbers, use the appropriate operator: if [ "$?" -ne 0 ]; then https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:53: echo "Failed to install $chrome_sandbox_installed_path" See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:58: if [ ! x$CHROME_DEVEL_SANDBOX = x"$chrome_sandbox_installed_path" ] See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:61: echo -n "CHROME_DEVEL_SANDBOX=$chrome_sandbox_installed_path\" " See above https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:63: echo "This variable is currently: ${CHROME_DEVEL_SANDBOX:-"empty"}" You don't need quotes around "empty"
https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:1: #!/bin/sh You can still do explicit error handling, even when using "-e". "-e" only has any effect, if the final exit code in a sequence of commands is false. In an "if false; then :; fi" command, the "false" is consumed by "if"; so, the shell won't exit on you. And the exit status of the "if" statement is the same as the exit status of its body. So, that would be "true" in this case. https://chromiumcodereview.appspot.com/10541017/diff/1/build/update-linux-san... build/update-linux-sandbox.sh:24: sudo -- cp $chrome_sandbox_build_path $chrome_sandbox_installed_path && You might want to warn users that you are about to run "sudo" for them -- and they might now get prompted for their password.
PTAL Thanks all. Markus: thanks for all the detailed explanations. In this specific script, I think I would really like to keep things simple, so I didn't implement some of your suggestions. Hopefully I took care of any bugs and utter ugliness though.
https://chromiumcodereview.appspot.com/10541017/diff/5004/build/update-linux-... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/5004/build/update-linux-... build/update-linux-sandbox.sh:11: # Make sure the path below is not on NFS! So do you want to stat /usr/local/sbin ? https://chromiumcodereview.appspot.com/10541017/diff/5004/build/update-linux-... build/update-linux-sandbox.sh:49: if [ "${CURRENT_API}" -ne "${INSTALLED_API}" ]; then nit: Unlike $?, these variables are not guaranteed to be numbers. So shouldn't this be != instead?
PTAL https://chromiumcodereview.appspot.com/10541017/diff/5004/build/update-linux-... File build/update-linux-sandbox.sh (right): https://chromiumcodereview.appspot.com/10541017/diff/5004/build/update-linux-... build/update-linux-sandbox.sh:11: # Make sure the path below is not on NFS! On 2012/06/06 07:39:33, Lei Zhang wrote: > So do you want to stat /usr/local/sbin ? This comment was more for someone modifying the code, but let's do it. https://chromiumcodereview.appspot.com/10541017/diff/5004/build/update-linux-... build/update-linux-sandbox.sh:49: if [ "${CURRENT_API}" -ne "${INSTALLED_API}" ]; then On 2012/06/06 07:39:33, Lei Zhang wrote: > nit: Unlike $?, these variables are not guaranteed to be numbers. So shouldn't > this be != instead? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jln@chromium.org/10541017/12002
Change committed as 140816 |
