Skip to content

Prevent truncating log tag name #83

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

Merged
merged 3 commits into from
Mar 4, 2025
Merged

Prevent truncating log tag name #83

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90f7f76
Prevent truncating log tag name
Feb 18, 2025
020dedf
Reduce LOGGING_TAG_MAX_LEN, add comments
Mar 4, 2025
e4da077
Make allocating the tag safe code
Mar 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 21 additions & 7 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,8 @@ impl AndroidLogger {

static ANDROID_LOGGER: OnceLock<AndroidLogger> = OnceLock::new();

const LOGGING_TAG_MAX_LEN: usize = 23;
// Maximum length of a tag that does not require allocation.
const LOGGING_TAG_MAX_LEN: usize = 127;
Comment on lines +182 to +183
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

const LOGGING_MSG_MAX_LEN: usize = 4000;

impl Default for AndroidLogger {
Expand Down Expand Up @@ -211,10 +212,10 @@ impl Log for AndroidLogger {
return;
}

// tag must not exceed LOGGING_TAG_MAX_LEN
// tag longer than LOGGING_TAG_MAX_LEN causes allocation
let mut tag_bytes: [MaybeUninit<u8>; LOGGING_TAG_MAX_LEN + 1] = uninit_array();

let module_path = record.module_path().unwrap_or_default().to_owned();
let module_path = record.module_path().unwrap_or_default();

// If no tag was specified, use module name
let custom_tag = &config.tag;
Expand All @@ -223,10 +224,23 @@ impl Log for AndroidLogger {
.map(|s| s.as_bytes())
.unwrap_or_else(|| module_path.as_bytes());

// truncate the tag here to fit into LOGGING_TAG_MAX_LEN
self.fill_tag_bytes(&mut tag_bytes, tag);
// use stack array as C string
let tag: &CStr = unsafe { CStr::from_ptr(mem::transmute(tag_bytes.as_ptr())) };
// In case we end up allocating, keep the CString alive.
let _owned_tag;
let tag: &CStr = if tag.len() < tag_bytes.len() {
// use stack array as C string
self.fill_tag_bytes(&mut tag_bytes, tag);
// SAFETY: fill_tag_bytes always puts a nullbyte in tag_bytes.
unsafe { CStr::from_ptr(mem::transmute(tag_bytes.as_ptr())) }
} else {
// Tag longer than available stack buffer; allocate.
// We're using either
// - CString::as_bytes on config.tag, or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// - str::as_bytes on record.module_path()
// Neither of those include the terminating nullbyte.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_owned_tag = CString::new(tag)
.expect("config.tag or record.module_path() should never contain nullbytes");
_owned_tag.as_ref()
};

// message must not exceed LOGGING_MSG_MAX_LEN
// therefore split log message into multiple log calls
Expand Down