Building git-core's ICS branch...

[erik360]

So I got this built last night to fix the ICS segmentation fault error and I had a few questions, if you don't mind.

1. When compiled, this binary is about 150K, versus about 20K for the master branch. Any idea why? I built both and the make files look the same.

2. It seems your gc branch is the most up to date, but does not include the segmentation fault / environment fix. Is there a plan to merge that or release a fixed su binary into the wild soon?

Thanks!

[git-core]

1. When compiled, this binary is about 150K, versus about 20K for the master
   branch. Any idea why? I built both and the make files look the same.
   git-core:ics (or chainsdd:gc-ics) was dirty experimental fix. Its
   whole purpose was to check whether it could help somebody. The code
   allocates a huge array statically, thus increased size of the
   executable.

2. It seems your gc branch is the most up to date, but does not include the
   segmentation fault / environment fix. Is there a plan to merge that or
   release a fixed su binary into the wild soon?
   When I asked ChainsDD last time, he though the patch isn't needed
   anymore. I don't have an ICS device, so my opinion doesn't count here.
   If you think the patch indeed is useful, please, explain why.

[erik360]

Thank you for the reply.

I have a Verizon Galaxy Nexus running AOKP b23 (4.0.3). The segmentation fault is definitely still an issue on ICS and ICS ports, failing on commands such as 'pm'. I follow zeppelinrox's SuperCharger thread on XDA ( http://forum.xda-developers.com/showthread.php?t=991276 ) and there have been other reports of the same errors including by zep himself. In fact he found the other open issue #6 discussing the problem on this git, which brought me here.

Both the mentioned workaround of passing LD_LIBRARY_PATH and your patch have fixed the issue on my phone and his (I believe he runs an ICS port on a Milestone). So yes, I think it's worth looking at this issue again.

A few more followup question, if you would indulge me. I'm new to AOSP and compiling against it. I followed your build instructions in your git wiki, so I believe I have the current 4.0.3 branch.

1. Is there any reason not to use your patch (in its current form) if it fixes the issue on ICS ROMs?

2. Against which AOSP target should I be building su? The one we've been testing I built against 'lunch full-user'.

3. Is there any reason the patched compile shouldn't work on pre-ICS ROMs such as CM7? I had one report of someone who said it 'didn't work' on CM7 (no more details yet), but I've yet to test it on my other phone, a Thunderbolt running a CM7 port (Thundershed 1.2). Does it matter which AOSP branch it's compiled against, or even which target?

Thank you again!

[git-core]

I've merged gc into gc-ics, so you may use the version w/o known bugs and security issues now.
Note, the merge is completely untested. (Well, it compiles at least.)

1. Is there any reason not to use your patch (in its current form) if it fixes the issue on ICS ROMs?
   As far as I understand the situation, ROM developers chose to modify the source of the bug/left it to App developers. Perhaps, I'm wrong.
   The patch in its current from is too heavy, I guess. It blindly copies all environment from the caller to the callee. The kernel allows up to 0x7FFFFFFF variables, 32 pages each (but not greater than 1/4 of stack size though) in the environment.
   If somebody would like to test, I could re-implement the patch by inheriting LD_LIBRARY_PATH only.
2. Against which AOSP target should I be building su?
   Doesn't matter for native binaries, I guess. I use to build against full-eng, for example. Official su-binary is compiled for armv6, but irrelevant of TARGET_PRODUCT/TARGET_BUILD_VARIANT.
3. Is there any reason the patched compile shouldn't work on pre-ICS ROMs
   I myself tested it on a 2.3 device.
   Does it matter which AOSP branch it's compiled against, or even which target?
   It doesn't until you're going to compile for Donut. Do you mean target in sense of the cpu arch like arm, x86, mips or in sense of the build variant of Android? If the former, su-binary is portable, I hope.

[erik360]

I'd like to test a version of your patch that inherits LD_LIBRARY_PATH only. It seems that is the only one that matters, so it would be nice if that's all it took to fix the problem.

Another question then... I've done a bit of searching and it seems that Android 4 dropped support for armv6. I'm guessing then it is not possible to compile su as armv6 with the current source tree? I see a post on androidsu.com from Oct. 23rd, 2011 that seems to hint at the same problem. It looks like ChainsDD found a way to compile for ARM generic. Was this with an old source tree, some other cross-compile method maybe?

[git-core]

I've pushed the patch into both chainsdd:gc-ics and git-core:ics repos.

It looks like ChainsDD found a way to compile for ARM generic

I don't know details but what I see in AOSP tree it is controlled by TARGET_PRODUCT, full sets generic as PRODUCT_DEVICE and, hence, chooses cpu arch and abis for armv7, generic_armv5 sets generic_armv5 (what a coincidence!) as PRODUCT_DEVICE and chooses default abis and armv5 as cpu arch.

[erik360]

I found the target setting and compiled for armv5, thanks for that!

However the patch doesn't seem to be working. After running su there is still no LD_LIBRARY_PATH environment string, and I get the seg faults.

Relevant logcat entries:

E/su (22700): sudb - Opening database
E/su (22700): sudb - Database opened
E/su (22700): sudb - Database closed
E/su (22700): Opening environment failed with 13: Permission denied

[git-core]

Got it. I forgot to test w/ non-root credentials. Could you test the following:
move call to populate_environment(ctx) from line 378 to line 535 where it shall look like
populate_environment(&ctx)
Thus, access to /proc//environ will be performed w/ root credentials.
(I assume seg fault occurs in the execed process, not in su itself, doesn't it?)

[perettigiuliano]

Sorry guys for intrusion i am following thread #6 because i am developing an app which send coomands to a su console but after upgrading my tablet to an ICS ROM this has stopped to work exactly with logcat repoted in thread #6.
What is the trick?

[git-core]

What is the trick?

The reason is explained there. The workaround is to pass LD_LIBRARY_PATH over su.

[perettigiuliano]

Odd... passing LD_LIBRARY_PATH as suggested there there is no errors in logcat but nothing happens...

[erik360]

move call to populate_environment(ctx) from line 378 to line 535 where it shall look like

I think our line numbers are a bit off. In the current su.c, the call to populate_environment is on line 377, and line 535 is in the middle a

if (setgroups(0, NULL)) {

block. Can you give me a line before / after marker to move that call? I'm thinking you may have meant move to line 545 after the three set* calls.

[erik360]

Ok, I fixed it. I moved the call to populate_environment(ctx) down about 9 lines, just above #define PARG(arg), and left it as is, not (&ctx). Tested with no errors or seg faults. I also verified LD_LIBRARY_PATH is set for the root shell. I just want to verify that's the proper patch?

[perettigiuliano]

Wraithdu nice done!
If you want a beta tester i am here.

[git-core]

I think our line numbers are a bit off.

I checked ChainsDD:gc-ics:su.c in the github source browser again. It shows populate_environment is on line 378. Oops, wait, this stupid piece of crap starts counting from line 5 actually. At least, for me. OK. You shall move populate code in main just before "if (setgroups(0, NULL)) {"
(after end of block of "if (chown(REQUESTOR_CACHE_PATH, st.st_uid, st.st_gid)) {")
The inserted line shall be
populate_environment(&ctx);
ctx is of type struct su_context in main() while it's pointer to struct su_context in allow(). Thus, the need of &.
Sorry, I still haven't enough time to fix the code myself.

I moved the call to populate_environment(ctx) down about 9 lines, just above #define PARG(arg)

It works only for cases when you switch to root. It doesn't work at all if you switch to a non-privileged user. If you may outlive this restriction you may keep your approach. The final patch will fix the bug differently, anyway. I'll rather add temporary identity change to populate_environment or near it and keep it exactly in the place it's being located now. But I'm not sure still.

[erik360]

Thanks for the clarification. I got your patch to work, so it's good for now until you finalize it.

Just a question... should you be able to 'su 1000' from a non-root shell? Or must you be root first, ie 'su; su 1000' ?