This is a direct port of the linux shell script.
NB. the lack of documentation surrounding functions should be addressed in both linux shell script and windows bat script. This is meant to be bug for bug compatible.
Details
Details
Diff Detail
Diff Detail
Event Timeline
| lib/asan/scripts/asan_device_setup.bat | ||
|---|---|---|
| 34 | I don't know the language at all, but some of this indents look strange. Could you double check? Ex. in the --lib case below, two SHIFTs are at different indentation levels. | |
| lib/asan/scripts/asan_device_setup.bat | ||
|---|---|---|
| 34 | spaces don't matter, so will try to space align it a bit better to add readability. Is there a way to do inline changes from the website or how do I submit the requested changes? | |
| lib/asan/scripts/asan_device_setup.bat | ||
|---|---|---|
| 34 | there is an "update diff" link in the upper right corner | |
Comment Actions
I have no skin in this game, so feel free to ignore me, but instead of forking the script like this, could we
- short term, make it compatible with something like the gnuwin32 version of bash
- long term, rewrite it in a more portable language
?
Comment Actions
Yes, (2) would be nice.
For example, the script has issues with reliability - adb can randomly fail, and the error rate goes up quickly with the number of connected devices. Some kind of retry logic will be added in the future, and of course the .bat version will not get it unless you or someone else comes up with a patch.
Comment Actions
I certainly would be game for rewriting this as a gradle plugin. It would seem easier to skip to step 2 in that regard, and wholly agree that a fork here isn't necessarily the best solution. However, I don't work for Google and thus have no idea what the requirements are for their use case (which might make a gradle plugin less than ideal). Thus I have no way to really determine which direction, or which language I'd want to make this head in. When I originally offered to do this port, I offered either a batch port of the existing bash file or a gradle plugin; assuming the gradle plugin could work in all places. Batch port was requested and doesn't require 3rd party tools installed that allow bash to run on windows.
I also would note that this is bug for bug compatible, meaning if the device doesn't have su installed then -use-su will fail in either platform with same messaging. I say that to mean that a lot of rooted android devices install sudo and leave off su; then again, end users are smart enough to install su if they have sudo. I thought supporting that, while nice, would violate it being a direct port.
Comment Actions
completely understood. It's up to you guys, am happy to offer keeping it updated as time allows. Although I'm more comfortable with bash and *nix than I am with windows. Lemme know what direction you guys want to take this in and I'll see what I can do to help.
Comment Actions
While I imagine gradle plugin would be useful for developers, we also need a command line utility that can be used in continuous testing setup.
Comment Actions
Python is a potential option? Although that gets into requiring third party tools/runtime. Lemme know if you have a language you'd recommend.
Gradle made sense to me as my assumption that all environments would have a JVM in some form so could support it and can be used from the command line via gradlew.
Comment Actions
Is windows more likely to have java runtime environment then python?
Python sounds like a great idea.
Another option is Go - it is about as expressive as Python, and it can generate standalone (i.e. static, no-libc) executables, and AFAIK it can cross-compile from linux to windows w/o sysroot or anything like that. The downside is that unlike a script, a binary can not be easily tweaked when necessary - this is something that I've had to do with the shell script on multiple occasions. And we'll need multiple binaries - linux/macos/windows and then there are non-x86 targets, too.
Maybe Python is a better choice. Is it reasonable to expect that an android developers' windows workstation has python runtime installed?
Comment Actions
Oh, how I wish I could say "yes, most windows devs have python installed." Unfortunately, It's highly unlikely to already be there. The only reason why it would be there from a pure Android developer perspective is to support working with MonkeyScript and then only a subset of those testers are going to have it as MonkeyScript uses Jython (JVM version of python). I think batch files are horrible for scripting purposes but Microsoft never really spent a lot of time focusing in on command line automation so many of our favorite tools just aren't available. JVM is the only real hard requirement to exist, in which case I'd venture gradle with maven is a better option. However, if you want something that just works on any windows machine, whether Android Studio is somewhere on the machine or only a standalone tool chain used from Unity/Unreal; then batch script is really the way to go which is what this commit is.
I've never touched Go, yet would be very eager to give that a try. Not that we couldn't create a pure static executable with good ole' C/C++; it isn't fun if it's not a challenge, and Go is new to me so would qualify as a challenge. :)
Comment Actions
I did have one other crazy thought. This might be entirely doable from a monkeyrunner script. You'd be pushing the requirement that the developers have Android SDK installed (which delivers both a monkeyrunner.bat and a monkeyrunner bash script. However, then you'd be able to rewrite this in python, and as a bonus eliminate a good portion of the function calls here as they are duplicated there. That might work.
Comment Actions
This is a pure python version. Tested on windows.
I also made a few changes to move the enforcing before the remount as android N seems to require it. Also this version will show command errors when they fail to work.
| asan_device_setup.py | ||
|---|---|---|
| 53 ↗ | (On Diff #107202) | Consider moving the implementation of basic actions (push, pull, shell, maybe install/chmod/chown) into a separate class (maybe Device), or at least grouping them near the beginning of the file. |
| 113 ↗ | (On Diff #107202) | This is hard to read. Also, each path is mentioned twice. Please build an array of paths and loop over it. |
| 118 ↗ | (On Diff #107202) | I think this line is missing "linux". |
| 184 ↗ | (On Diff #107202) | s/Zygot/Zygote/ |
| 191 ↗ | (On Diff #107202) | Move this into a helper function. |
| 259 ↗ | (On Diff #107202) | Is there a reason you are not using standard python argument parsing libraries? |
Comment Actions
As an aside, I have found a way to use ASan in devices using wrap.sh. So long as the device has the ability to use su or is a eng dev build the following wrap will work:
#!/system/bin/sh-from-zygote\n
ASAN_OPTIONS=start_deactivated=1,malloc_context_size=0
ASAN_ACTIVATION_OPTIONS=include_if_exists=/data/local/tmp/asan.options.b
LD_PRELOAD=libclang_rt.asan-${abi}-android.so
su -c setenforcing 0
$@
su -c setenforcing 1
I'm using it with this gradle:
static def writeWrapScriptToFullyCompileJavaApp(wrapFile, abi) {
if(abi == "armeabi" || abi == "armeabi-v7a")
abi = "arm"
wrapFile.withWriter { writer ->
writer.write('#!/system/bin/sh-from-zygote\n')
writer.write('ASAN_OPTIONS=start_deactivated=1,malloc_context_size=0\n')
writer.write('ASAN_ACTIVATION_OPTIONS=include_if_exists=/data/local/tmp/asan.options.b\n')
writer.write("LD_PRELOAD=libclang_rt.asan-${abi}-android.so\n")
writer.write('su -c setenforcing 0\n')
writer.write('\$@\n')
writer.write('su -c setenforcing 1\n')
}}
task createWrapScriptAddDir {
def libDir = file("$rootProject.ext.ndkDir").absolutePath + "/toolchains/llvm/prebuilt/"
if(System.properties['os.name'].toLowerCase(Locale.ROOT).contains('windows'))
libDir += "windows-x86_64/lib64/clang"
else
libDir += "*/lib*/*"
for (String abi : ["armeabi-v7a"]) {
def dir = new File("app/wrap_add_dir/" + abi)
dir.mkdirs()
def wrapFile = new File(dir, "wrap.sh")
writeWrapScriptToFullyCompileJavaApp(wrapFile, abi)
println "write file " + wrapFile.path
println "libdir = " + libDir
if(abi == "armeabi" || abi == "armeabi-v7a")
abi = "arm"
copy {
from fileTree(libDir).files
into dir.absolutePath
include "**/*asan*" + abi + "*.so"
}
}}
I'd note that with doing this; I'm not entirely sure there's still a need for this script to exist. I'm creating a blog post / tutorial on how to use ASan using the above wrap implementation; complete with example project. However, I'll make the suggested changes and resubmit.
| asan_device_setup.py | ||
|---|---|---|
| 259 ↗ | (On Diff #107202) | Not a particularly *good* reason. It's the same reasoning there's no helper class; this started its' life as a monkeyscript. Jython's parsing of args there was a bug so had to get around that. Force of legacy means it survived after I learned that python is used in other NDK scripts and thus would be safe to use everywhere; so it survived my refactor to pure python. |
| asan_device_setup.py | ||
|---|---|---|
| 1 ↗ | (On Diff #107202) | there is no "monkeyrunner" in linux |
| 56 ↗ | (On Diff #107202) | This needs either shell=True or the command as an array of strings. |
| 379 ↗ | (On Diff #107202) | This line needs to end with " \", or start with "export ", otherwise the environment variables are not passed to the exec'ed program. |
Comment Actions
This is great! Wrap.sh is definitely a better approach for 99% of users. Why are you saying that it needs an eng dev build though? I thought the idea behind wrap.sh is that it works on regular user devices, as long as the app has DEBUGGABLE in the manifest.
#!/system/bin/sh-from-zygote\n
ASAN_OPTIONS=start_deactivated=1,malloc_context_size=0
ASAN_ACTIVATION_OPTIONS=include_if_exists=/data/local/tmp/asan.options.b
LD_PRELOAD=libclang_rt.asan-${abi}-android.so
su -c setenforcing 0
$@
su -c setenforcing 1
As I mentioned in other comments, this script does not pass the variables to the child environment. It needs to be a one-liner or do explicit "export". Did you test it with actual ASan-instrumented code in the app?
I'd note that with doing this; I'm not entirely sure there's still a need for this script to exist. I'm creating a blog post / tutorial on how to use ASan using the above wrap implementation; complete with example project. However, I'll make the suggested changes and resubmit.
The only reason for the script to exist is performance - any wrap-based solution would restart the entire VM for every new process, which takes at least a few seconds.
| asan_device_setup.py | ||
|---|---|---|
| 1 ↗ | (On Diff #107202) | another case of surviving refactoring. I believe you might be able to find monkeyrunner in the toolchain Google distributes here: |
Comment Actions
It actually doesn't need to be an eng or dev build. I'm saying it needs root (setenforce 0 / setenforce 1 are the keys). If you have root, either through su or through adb root, then the wrap method will work.
#!/system/bin/sh-from-zygote\n
ASAN_OPTIONS=start_deactivated=1,malloc_context_size=0
ASAN_ACTIVATION_OPTIONS=include_if_exists=/data/local/tmp/asan.options.b
LD_PRELOAD=libclang_rt.asan-${abi}-android.so
su -c setenforcing 0
$@
su -c setenforcing 1As I mentioned in other comments, this script does not pass the variables to the child environment. It needs to be a one-liner or do explicit "export". Did you test it with actual ASan-instrumented code in the app?
Yep, tested it with a PixelXL and with emulator. Will add in the explicit export (was wondering why it didn't need it tbh).
I'd note that with doing this; I'm not entirely sure there's still a need for this script to exist. I'm creating a blog post / tutorial on how to use ASan using the above wrap implementation; complete with example project. However, I'll make the suggested changes and resubmit.
The only reason for the script to exist is performance - any wrap-based solution would restart the entire VM for every new process, which takes at least a few seconds.
That makes quite a bit of sense. alright, will refactor this script too. Thanks for the code review, these are some great ideas! Expect me to have a bit of time over the weekend to do it.
Comment Actions
Wait... egg on my face time. I sent you the script from my work machine. At home where I tested it the script looked like this (just looked at it):
#!/system/bin/sh-from-zygote\n
su -c setenforcing 0
ASAN_OPTIONS=start_deactivated=1,malloc_context_size=0 \
ASAN_ACTIVATION_OPTIONS=include_if_exists=/data/local/tmp/asan.options.b \
LD_PRELOAD=libclang_rt.asan-${abi}-android.so \
$@
su -c setenforcing 1
All is right with the world of linux shell scripting, my bug.
Comment Actions
I see. That's still bad, we'd like ASan to work on non-rooted devices. Do you know why setenforce 0 is necessary?
Comment Actions
completely agreed. I'd much prefer to not have to root.
See here for mystery reasoning of why (not a Google employee so not so easy for me to find out what the bugs are behind the scenes):
https://issuetracker.google.com/issues/63766481
It looks like it's at least a known issue. I've been told by other google employees that there's a plan to address this in the emulator in future revisions by having multiple zygote processes.
I don't know the language at all, but some of this indents look strange. Could you double check? Ex. in the --lib case below, two SHIFTs are at different indentation levels.