Opened 12 months ago

Last modified 4 months ago

#32181 needs_information defect

remove custom Android logging, use syslog

Reported by: eighthave Owned by: nickm
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: Android, 043-can, 044-deferred
Cc: n8fr8, ahf, sysrqb, sisbell Actual Points: .2
Parent ID: Points: 1
Reviewer: ahf Sponsor:

Description

As far as I can tell, syslog.h is fully supported for logging to Android's logcat. syslog.h is present as early as NDK r10e, if not earlier. It is also included on android-9 (2.3). My quick tests show syslog working fine on android-22 (5.1) through android-29 (10) emulators. android-24 (7.0) is currently the oldest version of Android supported by Google. syslog severity and identity tag are already mapped to the logcat versions.

@ahf is there some detail I'm missing? Were you having problems with syslog on Android?

Since the "android" logging option was never documented, I think it should just be removed, long with the add_android_log() code that was added in #24362 https://github.com/torproject/tor/commit/b0b8f7c30c8296344c77d06a24821f35b667e3dd

FYI, since syslog support is added about the same time as logcat support, #32036 is still an issue when using syslog with Android.

Child Tickets

Change History (17)

comment:1 Changed 12 months ago by nickm

We can deprecate direct calls to the logcat interface, or we can change "android" to be an alias for syslog, but we can't remove "android." We try very hard not to break code that uses Tor.

comment:2 Changed 12 months ago by eighthave

I've been working on Tor on Android for 10 years, and conducted a review of the Android apps that I know that include tor. None of them use this "android" log feature. Also, the "android" keyword was never documented. I think its safe to remove. But if you don't want to, just make "android" an alias for "syslog" and "AndroidIdentityTag" an alias for "SyslogIdentityTag".

comment:4 Changed 12 months ago by eighthave

In case it wasn't clear, logging to Log info android or Log info syslog produce the same messages to logcat, from what I've seen. That's one key reason why I think the code for the "android" logging method can be safely removed: it provides a duplicate code path ending in the same result.

comment:5 Changed 12 months ago by nickm

I'm still okay with removing the logcat code, but we do need to keep the deprecated option working for a couple of releases. We have a pretty firm policy that we try not to break working configurations without an extremely good reason. This policy might be annoying to work with some times, but it is a big part of what has kept us from breaking our downstream packages.

I believe you when you say that you've checked out all the Android apps that you know of that include Tor, but my experience with options is that there is always some super-obscure tool that we don't know about until we break it.

comment:6 Changed 12 months ago by eighthave

Fine by me if you want to keep it, as long as it remains undocumented and deprecated. No one should stuble upon it and try to use it, it just adds confusion. I myself wasted a chunk of time figuring out what was happening between the various logging methods, with the added pain of things like #32036

comment:7 Changed 12 months ago by eighthave

Here's a nice diagram and description of the Android logging system which could be helpful:
https://elinux.org/Android_Logging_System

Also, with Android, there could always be some weird edge case where the __android_log_printf() does something different than syslog, it is not a very

My personal preference when maintaining cross-platform systems is to use common APIs as much as possible that's where this bug report comes from. And I generally consider undocumented APIs as breakable, with good reason..

comment:8 Changed 12 months ago by teor

Keywords: 043-can added
Milestone: Tor: 0.4.3.x-final
Points: 1

comment:9 Changed 11 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:10 Changed 11 months ago by nickm

Actual Points: .2
Status: acceptedneeds_review

Branch is ticket32181 with PR at https://github.com/torproject/tor/pull/1594 .

I did find two references to "android logs" in the manual, though neither actually told you how to use them.

comment:11 Changed 10 months ago by eighthave

Looks good to me.

comment:12 Changed 10 months ago by dgoulet

Reviewer: ahf

comment:13 Changed 10 months ago by ahf

Status: needs_reviewneeds_information

One of the devices I used from Motorola back when we did S8 didn't send syslog messages from Tor to the adb logcat output. That was why this code was introduced in the first place.

Are we sure that none of the devices we want to support does the same thing? To be fair, this device was quite broken in many other ways, but was my lower bound for performance checks on Tor on Android.

comment:14 Changed 10 months ago by eighthave

Sigh, that's how the Android ecosystem can be, unfortunately. How old was that device? E.g. what Android version was it running? I don't think its worth spending much effort on Android older than 7.0 (android-24), since they don't even receive security updates from Google. I think dealing with quirks for older than 5.1 (android-22 released 2015) would not be worth spending any time.

If this happens with recent Android versions, it seems that logcat could be kept. I don't think @n8fr8 or I have encountered that since Orbot has always used syslog. There is always --Log "debug file /data/data/org.torproject.android/debug.log"

comment:15 in reply to:  14 Changed 10 months ago by teor

Replying to eighthave:

Sigh, that's how the Android ecosystem can be, unfortunately. How old was that device? E.g. what Android version was it running? I don't think its worth spending much effort on Android older than 7.0 (android-24), since they don't even receive security updates from Google. I think dealing with quirks for older than 5.1 (android-22 released 2015) would not be worth spending any time.

Tor's supported platforms policy rejects any OS that no longer gets security updates, with a few exceptions for common desktop / Tor Browser platforms:

https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/SupportedPlatforms#OSSupportlevels

comment:16 Changed 5 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

0.4.3 was released: Move non merge-ready 0.4.3 tickets to 044.

comment:17 Changed 4 months ago by nickm

Keywords: 044-deferred added
Milestone: Tor: 0.4.4.x-finalTor: unspecified

Bulk-remove tickets from 0.4.4. Add the 044-deferred label to them.

Note: See TracTickets for help on using tickets.