Prevent truncating log tag name #83

dextero · 2025-02-24T14:45:20Z

The currently hard-coded limit is 23 characters. This limit originates from the pre-Android 7.0 32-character limit on system property names, because Android's liblog allows setting the log level at runtime through log.tag system properties [1]. The limit meant that "log.tag.SOMETAG" must have been shorter than 32 bytes, leaving 23 bytes for the tag itself.

The system property length limitation was removed in [2], so since 2017 it no longer applies, and some Android components already use longer tags [3][4].

This change increases the size of the stack-allocated buffer used for null-terminating the tag to 128 bytes, and adds support for even longer tags by falling back to heap-allocated CString when the tag is longer than that. The fallback path will introduce a performance penalty, but at the same time will allow manual level adjustment via log.tag. system property regardless of the tag length.

[1] https://cs.android.com/android/platform/superproject/main/+/main:system/logging/logd/README.property;l=50?q=f:readme.property
[2] https://android-review.googlesource.com/c/platform/bionic/+/327226
[3] https://cs.android.com/android/platform/superproject/main/+/main:device/generic/goldfish-opengl/system/codecs/omx/plugin/GoldfishVideoDecoderOMXComponent.cpp;l=20
[4] https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/biometrics/fingerprint/2.2/default/BiometricsFingerprint.cpp;l=16

The currently hard-coded limit is 23 characters. This limit originates from the pre-Android 7.0 32-character limit on system property names, because Android's `liblog` allows setting the log level at runtime through `log.tag` system properties [1]. The limit meant that "log.tag.SOMETAG" must have been shorter than 32 bytes, leaving 23 bytes for the tag itself. The system property length limitation was removed in [2], so since 2017 it no longer applies, and some Android components already use longer tags [3][4]. This change increases the size of the stack-allocated buffer used for null-terminating the tag to 128 bytes, and adds support for even longer tags by falling back to heap-allocated CString when the tag is longer than that. The fallback path will introduce a performance penalty, but at the same time will allow manual level adjustment via log.tag. system property regardless of the tag length. [1] https://cs.android.com/android/platform/superproject/main/+/main:system/logging/logd/README.property;l=50?q=f:readme.property [2] https://android-review.googlesource.com/c/platform/bionic/+/327226 [3] https://cs.android.com/android/platform/superproject/main/+/main:device/generic/goldfish-opengl/system/codecs/omx/plugin/GoldfishVideoDecoderOMXComponent.cpp;l=20 [4] https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/biometrics/fingerprint/2.2/default/BiometricsFingerprint.cpp;l=16

dextero · 2025-03-04T09:13:07Z

@Nercury I'm not sure the PR triggered any notifications, and it seems you're the most active contributor to this repo. Would you find some time to review this (and also, rust-mobile/android_log-sys-rs#6 + #84)? It would be a great help in reusing those crates within AOSP.

Thank you in advance!

src/lib.rs

Nercury · 2025-03-04T09:21:28Z

This looks ok. It's not your responsibility to clean up overall buffer mess; let's leave that for later/another PR.

src/lib.rs

Nercury · 2025-03-04T09:48:35Z

Ok, please address the @MarijnS95 comments before merging.
Also, I looked again, and I don't get it why we are allocating string if we have a buffer that can contain the max-len tag already? It is basically unlimited-length now?

MarijnS95 · 2025-03-04T09:52:28Z

Also, I looked again, and I don't get it why we are allocating string if we have a buffer that can contain the max-len tag already? It is basically unlimited-length now?

Reading in between the lines, that's supported upstream now. It seems this change aims to increase the default buffer length a bit assuming most tags are a bit longer than the previous restrictive limit of 23 to reduce allocations, and fall back to a heap allocation if it's even bigger than the new "soft limit" of 128?

MarijnS95 · 2025-03-04T09:53:37Z

This change increases the size of the stack-allocated buffer used for null-terminating the tag to 128 bytes

@dextero note that there's a +1, so the stack-allocated buffer including NUL terminator is allowed to use 129 bytes:

android_logger-rs/src/lib.rs

Line 215 in d51b7ff

let mut tag_bytes: [MaybeUninit<u8>; LOGGING_TAG_MAX_LEN + 1] = uninit_array();

To use exactly 128-byte long stack allocated buffer.

Also remove unnecessary to_owned call, and clean up _owned_tag use.

MarijnS95 · 2025-03-04T12:47:18Z

src/lib.rs

+// Maximum length of a tag that does not require allocation.
+const LOGGING_TAG_MAX_LEN: usize = 127;


Nice opportunity for a doc-comment and a note regarding the NUL terminator being exempt from this limit?

Suggested change

// Maximum length of a tag that does not require allocation.

const LOGGING_TAG_MAX_LEN: usize = 127;

/// Maximum length(excluding NUL terminator) of a tag that does not require allocation.

const LOGGING_TAG_MAX_LEN: usize = 127;

MarijnS95 · 2025-03-04T12:52:29Z

src/lib.rs

+        } else {
+            // Tag longer than available stack buffer; allocate.
+            // We're using either
+            // - CString::as_bytes on config.tag, or


Ah, for config.tag it's a "little unfortunate" that we have to jump through these hoops by going from CString to &[u8] and back to CStr or CString and let those APIs or ourselves append a NUL terminator again.

Not sure if there's a clean way to handle that without making this very complex while dealing with record.module_path() as well 😅

That's an excellent observation! Please see the latest commit for an update that makes the happy-path of having a preconfigured tag not go through all this copy-to-buffer-and-then-reinterpret-as-cstr stuff.

Whoops, I didn't notice PR got merged in the meantime - I'll follow up with a cleanup.

MarijnS95 · 2025-03-04T12:54:20Z

src/lib.rs

+            // We're using either
+            // - CString::as_bytes on config.tag, or
+            // - str::as_bytes on record.module_path()
+            // Neither of those include the terminating nullbyte.


Technically record.module_path() returns a &str which may contain interior NULs (and never has a terminating NUL, that's reserved for CString/CStr) but it seems unlikely that log will ever return interior NULs here.

TIL again 😅 Putting nullbytes in the module path sounds pretty evil though.

I guess the current code does handle this case by doing a pointer transmute into CStr::from_ptr. I could do a similar thing here if that would make sense. WDYT?

If the module_path() included an interior NUL byte, the previous unsafe CStr::from_ptr() "hack" would have cut off the tag right there. The new implementation correctly panics, because it doesn't understand and doesn't know how to handle this malformed user input.

Note that there is a builder API that trivially allows someone to set the module_path to Some("hell\0there") for example: https://docs.rs/log/latest/log/struct.RecordBuilder.html#method.module_path

dextero · 2025-03-04T13:15:20Z

Also, I looked again, and I don't get it why we are allocating string if we have a buffer that can contain the max-len tag already? It is basically unlimited-length now?

Reading in between the lines, that's supported upstream now. It seems this change aims to increase the default buffer length a bit assuming most tags are a bit longer than the previous restrictive limit of 23 to reduce allocations, and fall back to a heap allocation if it's even bigger than the new "soft limit" of 128?

There is no fixed-size length limit for the tag now. When writing the log entry to logd daemon, the entire tag is passed as is.

The log reading API still has a limit of 5 * 1024 B, but that's on the entire "log entry" (tag + message + some metadata) .

The "soft limit" pointed out by @MarijnS95 is there to avoid creating huge stack-allocated buffers. I increased it to 128 as 23 was restrictive enough that I ran into truncated tags 😄

MarijnS95 · 2025-03-04T13:40:57Z

@dextero looking at those links and seeing liblog using iovec to write the strings consecutively makes one wonder why we're messing around with so much code and copies in android_logger-rs at all. If anything all of this extra allocation and copy-pasting overhead should be avoidable in most trivial cases when writing a Record, don't you think?

(Unless all of that is only available on newer Android versions / APIs of course)

Nercury · 2025-03-04T14:02:40Z

why we're messing around with so much code in android_logger-rs at all

I guess back then I wanted to split log messages that overflow max char limit on whitespace. Without allocations. Hence the complexity.

Nercury · 2025-03-04T14:04:56Z

I am going to merge this and other PRs and then do more thorough scan before publishing.

MarijnS95 · 2025-03-04T14:14:18Z

@Nercury not criticising what has been done so far, it's great to see the limitations from the API mapped out carefully to Rust 🥳! But now that we seem to want to support larger tags/messages here, and perhaps restrict (if needed via optional features / cfgs) to a newer Android API level in order to use simpler and less-constrained APIs, it might be advantageous to refactor.

EDIT: With new length-based APIs we might also get away without allocating nor copying bytes.

Nercury self-requested a review March 4, 2025 09:19

Nercury approved these changes Mar 4, 2025

View reviewed changes

MarijnS95 reviewed Mar 4, 2025

View reviewed changes

src/lib.rs Show resolved Hide resolved

MarijnS95 reviewed Mar 4, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

MarijnS95 reviewed Mar 4, 2025

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Marcin Radomski added 2 commits March 4, 2025 12:21

Reduce LOGGING_TAG_MAX_LEN, add comments

020dedf

To use exactly 128-byte long stack allocated buffer.

Make allocating the tag safe code

e4da077

Also remove unnecessary to_owned call, and clean up _owned_tag use.

MarijnS95 approved these changes Mar 4, 2025

View reviewed changes

Nercury merged commit 4280f2e into rust-mobile:master Mar 4, 2025
24 checks passed

dextero mentioned this pull request Mar 5, 2025

Avoid rewriting the preconfigured tag #87

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent truncating log tag name #83

Prevent truncating log tag name #83

dextero commented Feb 24, 2025

dextero commented Mar 4, 2025

Nercury commented Mar 4, 2025

Nercury commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025

MarijnS95 Mar 4, 2025

MarijnS95 Mar 4, 2025

dextero Mar 4, 2025

dextero Mar 4, 2025

MarijnS95 Mar 4, 2025

dextero Mar 4, 2025

MarijnS95 Mar 4, 2025

dextero commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025 •

edited

Loading

Nercury commented Mar 4, 2025

Nercury commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025 •

edited

Loading

		// Maximum length of a tag that does not require allocation.
		const LOGGING_TAG_MAX_LEN: usize = 127;

Prevent truncating log tag name #83

Prevent truncating log tag name #83

Conversation

dextero commented Feb 24, 2025

dextero commented Mar 4, 2025

Nercury commented Mar 4, 2025

Nercury commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025

MarijnS95 Mar 4, 2025

Choose a reason for hiding this comment

MarijnS95 Mar 4, 2025

Choose a reason for hiding this comment

dextero Mar 4, 2025

Choose a reason for hiding this comment

dextero Mar 4, 2025

Choose a reason for hiding this comment

MarijnS95 Mar 4, 2025

Choose a reason for hiding this comment

dextero Mar 4, 2025

Choose a reason for hiding this comment

MarijnS95 Mar 4, 2025

Choose a reason for hiding this comment

dextero commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025 • edited Loading

Nercury commented Mar 4, 2025

Nercury commented Mar 4, 2025

MarijnS95 commented Mar 4, 2025 • edited Loading

MarijnS95 commented Mar 4, 2025 •

edited

Loading

MarijnS95 commented Mar 4, 2025 •

edited

Loading