Use libc rather than iana-time-zone on Android.

[qwandor]

As discussed in #1018, this switches back to using localtime_r and mktime from libc to get the local timezone on Android, rather than depending on iana-time-zone and trying to parse Android's tzdata files directly. This is better because the tzdata file format is not stable, and we don't want to use iana-time-zone in the Android platform.

I'll send a separate PR to check for the availability of the new localtime_rz and friends and use them instead where possible.

[djc]

So I'm honestly not sure after the latest discussion in #1018 whether I still want to go with using localtime_r() and mktime(), given that they're still not really safe.

[pitdicker]

Is this summary correct?

The current solution in chrono is fine for most users, and avoids localtime_r for which we got a security advisory in the past. This PR wants to reintroduce using localtime_r.

Reason is the use of chrono in the android project. The dependency iana-time-zone is not imported there, because it in turn depends on android_system_properties. That one is not acceptable because it uses an unstable API, and duplicates the functionality of rustutils. But iana-time-zone can't switch to rustutils as it is not available on crates.io.

A second reason is that the file format used for the timezone database on android is not guaranteed to be stable. It seems to me the benefits of being able to make use of the OS timezone database instead of shipping a database with an application, which can get outdated, outweigh the potention breakage in the future. It should not be too hard to update the parser to the new format.

Maybe the use of localtime_r should be behind an off-by-default feature with a scary name, like unsafe_android_libc?

[djc]

Yeah, I think your summary is correct. On top of that, future versions of Android might come with a safer libc API to get at the timezone offset data.

Maybe the use of localtime_r should be behind an off-by-default feature with a scary name, like unsafe_android_libc?

Seems like a reasonable solution.

[qwandor]

I've updated this PR to include the code to try at runtime to find the new localtime_rz and friends and use them if they are available. If not, it will fall back to localtime_r and mktime. I've tested this both on a nightly build of Android which has the new functions available, and an older build which doesn't. Both work as expected.

I disagree that "the current solution in chrono is fine for most users". Use of the system property as an API is not great, but depending on the format of the timezone database is worse (especially for applications outside of the platform) as it could change any time the database is updated, which happens fairly regularly. Suppose that a bunch of users have installed an application using chrono to handle timezones. The application works fine, until at some point in the future some country suddenly changes their daylight savings time rules in a way which requires new code to handle some new logic to handle. Android ships an update to the system timezone database, along with new parser code to handle it. All the applications using the supported APIs continue to work fine, but the application using chrono suddenly starts failing. To the users, it looks like Android just suddenly broke the application, but really the problem is that the application was using an unstable internal implementation detail rather than a supported API.

As far as I know it's not possible with cargo to make adding a feature remove a dependency, like the suggested unsafe_android_libc feature above. I guess we could do it the other way around, and have a feature like android_use_unsupported_internal_api to go back to the current approach of parsing the tzdata files, but the more different code paths we have the harder this is to test, and I don't think there's any situation where that is the right thing to do.

[djc]

I've updated this PR to include the code to try at runtime to find the new localtime_rz and friends and use them if they are available. If not, it will fall back to localtime_r and mktime. I've tested this both on a nightly build of Android which has the new functions available, and an older build which doesn't. Both work as expected.

That sounds great!

I disagree that "the current solution in chrono is fine for most users". Use of the system property as an API is not great, but depending on the format of the timezone database is worse (especially for applications outside of the platform) as it could change any time the database is updated, which happens fairly regularly.

I agree that the current situation is not great. But, Android is the platform here that has chosen to deviate from standard ways of exposing the current timezone and system timezone database. You seem to make an argument that the platform can ship a new system timezone database and new parser code to handle potential format changes in it -- if you can ship that, why can't you also ship APIs that expose that data to userspace with a better API at the same time? That would also solve this problem AFAICT.

(Shipping the timezone database in a format that regularly has to change to accomodate updates seems problematic to me -- why is this format better than the format which all other Linux distributions use, whose format has changed ~never?)

I guess we could do it the other way around, and have a feature like android_use_unsupported_internal_api to go back to the current approach of parsing the tzdata files, but the more different code paths we have the harder this is to test, and I don't think there's any situation where that is the right thing to do.

Adding a feature like that seems reasonable to me. I don't think this would have to lead to some kind of combinatorial explosion of code paths, as you seem to suggest.

[qwandor]

I disagree that "the current solution in chrono is fine for most users". Use of the system property as an API is not great, but depending on the format of the timezone database is worse (especially for applications outside of the platform) as it could change any time the database is updated, which happens fairly regularly.

I agree that the current situation is not great. But, Android is the platform here that has chosen to deviate from standard ways of exposing the current timezone and system timezone database. You seem to make an argument that the platform can ship a new system timezone database and new parser code to handle potential format changes in it -- if you can ship that, why can't you also ship APIs that expose that data to userspace with a better API at the same time? That would also solve this problem AFAICT.

On talking more to people internally about this, it sounds like I was partly wrong. Android doesn't generally make changes to the format and parser in mainline updates, usually just between releases. Still, the format is not guaranteed to be stable. And developers of Android applications probably want them to work correctly on devices running a range of different Android releases without having to rebuild them.

Android does expose APIs to userspace to access the timezone data. The main ones are the Java APIs, which are not very helpful here unless we add a bunch of JNI code to chrono, which seems like something it would be best to avoid. The supported native APIs up to API level 34 are localtime_r and friends. Adding a new API requires a new release, so we've added tzalloc and localtime_rz and so on in API level 35, which will be sometime after Android 14. I'd be happy for chrono to just use these functions, and that would be fine for Rust code in the platform, but application developers probably want their apps to work on older versions of Android too, without having to have different code paths for each version, which is why I've kept the fallback to localtime_r and mktime.

(Shipping the timezone database in a format that regularly has to change to accomodate updates seems problematic to me -- why is this format better than the format which all other Linux distributions use, whose format has changed ~never?)

From what I understand there are changes in other Linux distributions too. The syntax of the format is stable, but the semantics (i.e. what gets filled in and the meaning of what's there) is less stable.

[djc]

If the format is usually not changed between mainline updates, I think that means we're safe if we (a) keep the current strategy for current/old Android versions and (b) use the new localtime_rz stuff for newer platforms, right? Anecdotally the current strategy is working for Android versions that people are using chrono on, and as the new libc APIs roll out we'll transparently upgrade to a stable supported libc API.

[qwandor]

I've added a feature, PTAL. Unfortunately it doesn't seem to be possible with cargo to completely avoid the iana-time-zone dependency without the feature, because it's still used unconditionally on other targets, so we'll still need a downstream patch for that. We can at least avoid the android-tzdata dependency though.

[Kijewski]

I'm a maintainer of iana-time-zone. What I would like to know is: Why aren't you reaching out to android_system_properties? The library is used in 44.5k projects, so if you found some usage of restricted and/or unstable API, then why are you so vague in calling out the problems? The library is really small and well documented.

The library could be much smaller if Rust had weak symbols or if it dropped support for Android < 5. The former won't happen anytime soon. The latter would be a downgrade. (I guess you guys want to sell more new phones, so maybe it would be an upgrade to you.) ;) Your rustutils in fact does not seem to support Android < 5, does it?

[djc]

I'm a maintainer of iana-time-zone. What I would like to know is: Why aren't you reaching out to android_system_properties? The library is used in 44.5k projects, so if you found some usage of restricted and/or unstable API, then why are you so vague in calling out the problems? The library is really small and well documented.

Did you already read all of the preceding discussion in #1018?

[Kijewski]

I did. I realize that the discussion was about the format of Android's tzdata, too, not only android_system_properties, but the usage of an unstable API function was mentioned why android_system_properties could not be used in their projects. Yes, __system_property_get() should not be used anymore, but it is only used as a fallback if __system_property_read_callback() was not found.

I found #1018 quite odd. In my book "TL;TR: It uses unstable API" does not count as an answer. Multiple questions about the unstable API weren't (properly) answered. In my not-so-humble opinion it's a bad style from the googlers, that they didn't try to contact the downstream libraries to maybe work with them, because I think it would be in their best interest, too.

(Actually, I was only looking if #1129 was resolved, and was only looking through the issues because of that. Then the title of the PR just caught my eye, whether there was something that could be fixed/enhanced in iana-time-zone, etc.)

[qwandor]

I'm a maintainer of iana-time-zone. What I would like to know is: Why aren't you reaching out to android_system_properties? The library is used in 44.5k projects, so if you found some usage of restricted and/or unstable API, then why are you so vague in calling out the problems? The library is really small and well documented.

The library could be much smaller if Rust had weak symbols or if it dropped support for Android < 5. The former won't happen anytime soon. The latter would be a downgrade. (I guess you guys want to sell more new phones, so maybe it would be an upgrade to you.) ;) Your rustutils in fact does not seem to support Android < 5, does it?

There are two distinct cases that have somewhat different concerns: Rust code in the Android platform, and Rust code in Android applications. I work on the platform so my primary concern is with that, and the immediate issue I'm trying to solve is that we are blocked on updating the version of chrono we vendor into the platform. I would also like Android applications to be using the correct APIs, so I'm trying to help with that where I can.

rustutils as it stands was only intended for use in the platform and so doesn't support old Android API versions. We are working on an external version that will be suitable for usage in applications as well, but that's a separate issue. That said, Google Play policies require app updates since November last year to target Android 12 (or Android 11 for Wear OS), so supporting versions older than 5 seems like it may be unnecessary.

The issue with android_system_properties isn't (as far as I know) that the functions it calls are unstable, but that system properties in general are not part of the stable API intended for applications to use. Their names or values could change in unexpected ways, so applications should use supported API functions rather than accessing system properties directly.

[Kijewski]

@qwandor, thank you for the explanation!

[pitdicker]

Use of the system property as an API is not great, but depending on the format of the timezone database is worse (especially for applications outside of the platform) as it could change any time the database is updated, which happens fairly regularly. … The application works fine, until at some point in the future some country suddenly changes their daylight savings time rules in a way which requires new code to handle some new logic to handle. Android ships an update to the system timezone database, along with new parser code to handle it.

Android is just using concatenated TZif files if I understand it right, of which the format is just like Linux, macOS and the BSDs. There is RFC 8536 to describe the format. If worse comes to worse and there is some rule for future dates that can't be expressed, current practice is to just fall back to a long list of transition dates. This already happens for countries that adjust the clock around ramadan, which depends on the moon. I don't see the format breaking anytime soon.

[pitdicker]

The nice thing about doing the calculations in chrono is that we can improve over what libc offers:

• We can return the offset abbreviation, as in https://github.com/chronotope/chrono/pull/750 (to fix issue https://github.com/chronotope/chrono/issues/749).
• We can return LocalResult::Ambiguous if a datetime falls in a transition fold.
• We can return LocalResult::None if a datetime falls in a transition gap, instead of a bogus value.
• We may (in a future release) tell what the surrounding valid dates are if a datetime falls in a transition gap.

[pitdicker]

From what I understand there are changes in other Linux distributions too. The syntax of the format is stable, but the semantics (i.e. what gets filled in and the meaning of what's there) is less stable.

As far as I know they can choose whether to include backzone, and the range of dates to include a fast lookup for, before depending on TzRule.

[pitdicker]

I work on the platform so my primary concern is with that, and the immediate issue I'm trying to solve is that we are blocked on updating the version of chrono we vendor into the platform. I would also like Android applications to be using the correct APIs, so I'm trying to help with that where I can.

@qwandor Just want to say that although I am doing difficult and don't like the direction, I appreciate your work.

[pitdicker]

That is the current status of this PR?

[qwandor]

I added a feature flag in 3923d5c to revert to the old behaviour, this is awaiting review.

[pitdicker]

Reintroducing CVE-2020-26235/RUSTSEC-2020-0159, which caused chrono a lot of pain (and moved a good number of users to time-rs which was earlier with a kind of fix), is not something to quickly step over. I still think the motivation 'we don't want to use iana-time-zone in the Android platform' is not strong enough.

With the changes to the documentation and the following discussion, keep in mind that for us it reads like: reintroduce CVE on android to avoid using an unstable API. I have to admit both are not great places to be in.

If the format is usually not changed between mainline updates, I think that means we're safe if we (a) keep the current strategy for current/old Android versions and (b) use the new localtime_rz stuff for newer platforms, right? Anecdotally the current strategy is working for Android versions that people are using chrono on, and as the new libc APIs roll out we'll transparently upgrade to a stable supported libc API.

If we go with this approach this PR could make easier progress.

[pitdicker]

I've opened an issue for using localtime_rz on Android if available. Closing this PR.