8000 Fix some failures by yan12125 · Pull Request #10 · rave-engine/python3-android · GitHub
[go: up one dir, main page]
Skip to content

Fix some failures #10

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 24, 2015
Merged

Fix some failures #10

merged 3 commits into from
Nov 24, 2015

Conversation

yan12125
Copy link
Contributor

This PR contains two parts:

  1. fix platform.android_version()
    On my first phone subprocess.check_call(['/system/bin/getprop', 'ro.build.version.release']) returns empty string b'', while in adb shell, /system/bin/getprop ro.build.version.release returns '4.1.1'. I don't know why but I try to make the falling back part working.

Then it's the problem of binary/unicode. My /system/build.prop contains the following line:

ro.build.date=五  3月 15 13:39:06 CST 2013

Non-ascii characters forces me to read in binary data first and then explicitly decode them.

Finally on my another phone /system/build.prop contains the following line:

dalvik.vm.dexopt-flags=m=y

As a result, I need the maxsplit parameter.
2. Correct libm.so dependency in selectmodule.c
Since cpython hg 88854, Modules/selectmodule.c contains a ceil call. Without explicitly linking to libm.so, import select fails.

With the above two fixes, I can run youtube-dl with the following command:

python3 youtube-dl -v --no-check-certificate mugIxP3LFhw

Tested environments:

  1. HTC butterfly x920d with official 4.1.1 ROM
  2. Samsung Galaxy Note N7000 with my own CyanogenMod 11.0 build, Android 4.4.4

yan12125 8000 added 2 commits November 22, 2015 00:41
@yan12125
Link select.cpython-34m.so with libm.so
ee73692 
ceil is introduced since cpython hg rev 88845:827d948ac6aa
@yan12125
Correctly handling in android_version()
969dbfa
@yan12125
Copy link
Contributor Author

Finally I find out why getprop in subprocess.Popen fails - on Android 4.1.1 properties are read from a inherited file descriptor. See Android 4.1.1's __system_properties_init for more information. The patch is updated (forcibly) according to my discovery.

@shizmob
Copy link
Member
shizmob commented Nov 24, 2015

Thanks for finding and diagnosing these issues!
It mostly looks good to me, however, given that these property values are internationalized, wouldn't decoding them as utf-8 (which I presume /system/build.prop is encoded in) instead of ascii be a better approach?

@yan12125
Copy link
Contributor Author

In fact in C level there's no assumption that property values should be valid UTF-8 strings. I can create a malicious /system/build.prop:

...
00000170: 670a 726f 2e62 7569 6c64 2e75 7365 723d  g.ro.build.user=
00000180: 7999 6e0a 726f 2e62 7569 6c64 2e68 6f73  y.n.ro.build.hos
...

(Simply replace my name b'yen' with b'y\x99n')
getprop prints its 'value':
screenshot_2015-11-25_03-35-05

And python script fails:

shell@GT-N7000:/data/local/tmp $ cat getprop2.py                                                             
import subprocess

p = subprocess.Popen(['getprop', 'ro.build.user'], stdout=subprocess.PIPE, stderr=subprocess.PIPE, pass_fds=(9,))
out, err = p.communicate()
print(out)
print(out.decode('utf-8'))
shell@GT-N7000:/data/local/tmp $ python3 getprop2.py                                                           
b'y\x99n\n'
Traceback (most recent call last):
  File "getprop2.py", line 6, in <module>
    print(out.decode('utf-8'))
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x99 in position 1: invalid start byte

In Java land, both keys and values of system properties are assumed UTF-8 (see the JNI part of android.os.SystemProperties). Malformed property values cause the Dalvik VM crashed with SIGABRT.

As a result, I think it's OK to use UTF-8 everywhere as of now. Just to mention it as a record. If these codes are eventually merged into CPython reposiroty some day, I or someone else may want to propose such a possible case.

@shizmob
Copy link
Member
shizmob commented Nov 24, 2015

Looks good to me; if Java assumes this, it seems fairly canonical and safe for us to assume as well. If nothing else, it's better than just decoding as ASCII, which would have similar issues.

shizmob added a commit that referenced this pull request Nov 24, 2015
@shizmob
Merge pull request #10 from yan12125/master
6a55e2c 
Fix Android property retrieval and select module libm dependency.
@shizmob shizmob merged commit 6a55e2c into rave-engine:master Nov 24, 2015
@shizmob
Copy link
Member
shizmob commented Nov 24, 2015

Again, thanks for reporting, diagnosing and fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yan12125 @shizmob
0