vidstige / jadb

ADB Client in pure Java.
Apache License 2.0
640 stars 178 forks source link

execute command not working with mutated vowels (german umlaute) #134

Closed root-intruder closed 3 years ago

root-intruder commented 3 years ago

I found out, that the JadbDevice execute command doesn't work with mutated vowels (üäö)

execute("echo", "sadöok") delivers: sadöo NOTE: last letter missing

the same with a space reacts different since the executor wraps the argument in ' execute("echo", "sa döok") delivers: /system/bin/sh: no closing quote

I'll look into if I'm able to solve this myself

root-intruder commented 3 years ago

fixed in #135 these characters can not be escaped and should be either filtered out or a exception shall be thrown. I went for the second option to not silently introduce errors without users noticing

vidstige commented 3 years ago

Yes, the Bash.quote is a bit bugged. It does not handle all characters well. However, this probably does not have to do with the bash-quoting. Perhaps utf-8 encoding is not used everywhere properly? :thinking: It would be interesting to add a unit test that executes a command containing e.g. umlauts. I'm guessing there is a reader or similar somewhere that does not have utf-8 encoding.

vidstige commented 3 years ago

Perhaps add a test case similar to this one and see if you can find anything? https://github.com/vidstige/jadb/blob/master/test/se/vidstige/jadb/test/unit/MockedTestCases.java#L99

root-intruder commented 3 years ago

my commit includes testcases for umlauts.

executing the test without the bash quoting fix will pass the string "sa döok" correctly to the output stream https://github.com/vidstige/jadb/blob/3e2a88faf10dc0e1fb4dd9d1a91a7492044c3e7d/src/se/vidstige/jadb/Transport.java#L58

which is correctly setup as UTF-8. however after that comes the adb protocol which then messes up with any character not in the ASCII range between 32-126. And I think there is nothing we can change since this output stream is the last element in out hands. Thus we should take care to not pass such characters to adb.

root-intruder commented 3 years ago

@vidstige, I updated my merge request

root-intruder commented 3 years ago

resolved with merge #135