Alter the AppExitInfoService to encode NDK protobuf info in escaped chars #22

fractalwrench · 2023-10-23T14:08:01Z

Goal

This alters the AppExitInfoService to encode AEI blobs as Base64 in the payload.

When I attempted to read the NDK protobuf file it became apparent that there was an issue with how data is recorded in the blob payload. The ANR trace file is UTF-8 text so is unaffected, but the NDK crash protobuf file is not.

The SDK was reading the file as UTF-8 text, using BufferedReader::readText. I think that this resulted in unreadable characters being replaced with \UFFFD by the CharsetDecoder, and destroyed any possibility of us being able to read the NDK crash protobuf that was sent to the backend.

There are two approaches we could take to fix this:

Base64 encode the information
Update the blob message endpoint to accept multipart requests, and send the AEI blob as a binary part

I have not altered the payload keys etc, which we would probably want to do as part of this.

Testing

I generated a Java class using protoc from the tombstone schema. I then saved an NDK crash that was saved as part of AEI. A unit test was then added to simulate parsing the protobuf at different stages of its journey through the system:

from a local file
after reading the local file into a Java object
after serializing/deserializing JSON

fnewberg · 2023-10-24T00:14:11Z

...src/main/java/io/embrace/android/embracesdk/capture/aei/EmbraceApplicationExitInfoService.kt

 logDebug("AEI - No info trace collected")
 return null
 }
+ val trace = encoder.encodeToString(bytes)


i talked to @noonat about this and he thought there was a way to do this without restoring to base64 encoding. i'll let him chime in with his suggested approach

base64 compresses poorly, so it would be good if we can avoid using it.

cc: @pablomatiasgomez

My thought was that JSON strings do support arbitrary binary values with \uXXXX escape sequences, and that if the binary data could instead be converted into a valid Java string then it might be encodable as JSON using the escape sequences where necessary. The \uXXXX sequences in JSON strings are 6 bytes each, so this would only work out in practice if the string is mostly made up of valid ASCII characters, as @fnewberg was suspecting.

I haven't been able to find any way to do this in standard library Java/Kotlin, as all the encoding/decoding functions seem to expect the byte sequences to conform to a codec of some sort. But you could do something like this:

fun bytesToUTF8String(bytes: ByteArray): String { val encoded = ByteArray(bytes.size * 2) var i = 0 for (b in bytes) { val u = b.toInt() and 0xFF if (u < 128) { encoded[i++] = u.toByte() continue } encoded[i++] = (0xC0 or (u shr 6)).toByte() encoded[i++] = (0x80 or (u and 0x3F)).toByte() } return String(encoded.copyOf(i), Charsets.UTF_8) }

If you take the resulting string and dump it to JSON with Gson you get something with \uXXXX escape sequences in it. I attempted encoding a byte array of the values 0 to 255 using this method and JSON decoding it on the Go side and it does seem to work fine (although you have to iterate over the string and convert the runes to bytes manually).

@noonat do you think this is the preferred approach over base64? should we produce a payload both ways then try compressing it with gzip to see how the sizes compare?

pretty easy to get a basic comparison with python

>>> import gzip >>> with open('/Users/fredric/Downloads/aei-trace', 'rb') as fd: ... data = fd.read() ... >>> len(data) 341238 >>> compressed = gzip.compress(data) >>> len(compressed) 59755

Sure, I will do that and reply with the results here.

Okay, I wrote a Go script to encode as base64 and utf8, marshal both to JSON, then compress with gzip. These are the sizes:

unencoded: 341238 base64: 104184 utf8: 69129

It also decoded and compared the resulting buffers to the original buffer to make sure nothing weird is happening.

You can toString() a ByteArray: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/to-string.html

I am away from a computer, but let me verify that toString actually works properly. When I was trying some of those techniques before, I ended up with Unicode replacement characters in the output (as the conversion functions expected the byte stream to already be a valid UTF-8 stream). But I can't remember if I tried toString.

toString() takes a CharSet so I'd be surprised if it didn't work. Now, how well it performs... that I don't know 😅

Right -- but it takes a charset that is expected to express the format of the incoming bytes. e.g. if you have a stream of UTF-8 bytes and you want to convert that in memory into a Java string representation, you specify the charset of the byte stream as UTF-8 so that toString knows how to interpret it. If it encounters bytes that don't conform to the encoding, it replaces them with Unicode replacement characters in the resulting string.

You can think of the charset on toString and the String constructor as instructing what format to use when parsing the raw bytes into a string. It's not expressing what format to convert the byte stream into, which is what we need.

As an example, if I create a byte array containing bytes 247 through 255:

var b = byteArrayOf() for (i in 247..255) { b += (i).toByte() }

If I convert them with bytes.toString(Charsets.UTF_8) and print out the JSON string and the .toByteArray().contentToString(), I get this:

"��" [-17, -65, -67, -17, -65, -67, -17, -65, -67, -17, -65, -67, -17, -65, -67, -17, -65, -67, -17, -65, -67, -17, -65, -67, -17, -65, -67]

Note that all the characters have been replaced with the same Unicode replacement character. If I instead use this custom encoding function, I get this:

"÷øùúûüýþÿ" [-61, -73, -61, -72, -61, -71, -61, -70, -61, -69, -61, -68, -61, -67, -61, -66, -61, -65]

...d-sdk/src/test/java/io/embrace/android/embracesdk/capture/aei/AeiNdkCrashProtobufSendTest.kt

fractalwrench · 2023-10-25T11:39:54Z

@noonat @fnewberg I've updated the changeset to encode the AEI blob with the suggested approach of escaping characters.

It's worth noting that currently this applies to both the ANR + NDK blobs. I assume we'd need to update the backend to handle this encoding change before we ship this as part of an SDK release?

bidetofevil

I'll let Fredric/Nathan finish up the review regarding the encoding stuff, this this looks good to me. And I'd probably just use the toString() method on the ByteArray unless this implementation is more efficient?

bidetofevil · 2023-10-25T13:03:52Z

buildSrc/src/main/kotlin/io/embrace/gradle/InternalEmbracePlugin.kt

@@ -153,7 +153,7 @@ class InternalEmbracePlugin : Plugin<Project> {
 testOptions {
 unitTests {
 all { test ->
- test.maxParallelForks = (Runtime.getRuntime().availableProcessors() / 2) + 1
+ test.maxParallelForks = (Runtime.getRuntime().availableProcessors() / 3) + 1


Was it deadlocking that caused your test to run for 2 days? ;-)

The protobuf file added as part of the tests in this changeset seems to be failing CI consistently due to OOMs, and failed locally for me once or twice. I think reducing the number of parallel forks should help mitigate this, or perhaps only just using one test process

Weird. Leaks? Or maybe the heap size was too small? I'm surprised the file was so big that it runs into problems if there were no leaks or if the heap is the right size.

bidetofevil · 2023-10-25T13:09:06Z

...src/main/java/io/embrace/android/embracesdk/capture/aei/EmbraceApplicationExitInfoService.kt

 logDebug("AEI - No info trace collected")
 return null
 }
+ val trace = encoder.encodeToString(bytes)


You can toString() a ByteArray: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/to-string.html

noonat · 2023-10-25T15:40:07Z

I assume we'd need to update the backend to handle this encoding change before we ship this as part of an SDK release?

Yes, although I suspect it will be backwards compatible for the ANR blobs, since those are already text and should not normally be effected by the encoding step.

fractalwrench · 2023-10-25T16:11:48Z

I will update this so that the encoding only applies to AEIs with REASON_CRASH_NATIVE - that should prevent ANR blobs from being affected

fnewberg · 2023-10-25T16:49:40Z

@noonat @fnewberg I've updated the changeset to encode the AEI blob with the suggested approach of escaping characters.

It's worth noting that currently this applies to both the ANR + NDK blobs. I assume we'd need to update the backend to handle this encoding change before we ship this as part of an SDK release?

@noonat i spoke with @fractalwrench about this and we discussed a couple of things:

the ANR data should not be impacted in any way since it has no binary content
we should be able to look at the reason value and only use this way of reading and encoding the data for crash_native reason values

@fractalwrench LMK if i missed something there

i don't think we will need to change any keys in the payload, e.g. change blob to blob2, since the ANR data should not be impacted and the protobuf data is broken today anyways

...src/main/java/io/embrace/android/embracesdk/capture/aei/EmbraceApplicationExitInfoService.kt

fractalwrench · 2023-10-27T11:24:42Z

I've updated this changeset so that AEI traces are only encoded for native crashes on API >=31, and added some extra test coverage.

I will see what happens with the CI run & possibly look into increasing the runner size, or decreasing the parallel forks used for running tests on CI only.

codecov · 2023-10-27T11:35:04Z

Codecov Report

Merging #22 (64e9e98) into master (c075898) will decrease coverage by 0.02%.
The diff coverage is 61.29%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #22      +/-   ##
==========================================
- Coverage   72.66%   72.64%   -0.02%     
==========================================
  Files         305      305              
  Lines        9986    10012      +26     
  Branches     1440     1446       +6     
==========================================
+ Hits         7256     7273      +17     
- Misses       2161     2166       +5     
- Partials      569      573       +4

Files	Coverage Δ
...k/capture/aei/EmbraceApplicationExitInfoService.kt	`82.96% <61.29%> (-4.20%)`	⬇️

... and 1 file with indirect coverage changes

bidetofevil

Looks good!

$fractalwrench$

$@fractalwrench$ fractalwrench requested a review from fnewberg October 23, 2023 14:08

fnewberg reviewed Oct 24, 2023

View reviewed changes

$@fractalwrench$ fractalwrench marked this pull request as ready for review October 25, 2023 11:39

$@fractalwrench$ fractalwrench requested a review from a team as a code owner October 25, 2023 11:39

$@fractalwrench$ fractalwrench changed the title ~~[WIP] Alter the AppExitInfoService to encode AEI blobs as Base 64 in the payload~~ Alter the AppExitInfoService to encode NDK protobuf info in escaped chars Oct 25, 2023

bidetofevil reviewed Oct 25, 2023

View reviewed changes

noonat approved these changes Oct 25, 2023

View reviewed changes

...src/main/java/io/embrace/android/embracesdk/capture/aei/EmbraceApplicationExitInfoService.kt Outdated Show resolved Hide resolved

fractalwrench added 3 commits October 27, 2023 11:30

$@fractalwrench$

test: add test for reading protobuf from AEI object

f24b3ed

$@fractalwrench$

encode AEI strings

ca9f665

$@fractalwrench$

reduce test fork count due to large class loading

64e9e98

$@fractalwrench$ fractalwrench force-pushed the fix-ndk-tombstone-capture branch from 3b14b1f to 64e9e98 Compare October 27, 2023 11:22

$@fractalwrench$ fractalwrench requested a review from bidetofevil October 27, 2023 12:54

bidetofevil approved these changes Oct 30, 2023

View reviewed changes

$@fractalwrench$ fractalwrench merged commit cc08fa6 into master Oct 31, 2023
3 of 4 checks passed

$@fractalwrench$ fractalwrench deleted the fix-ndk-tombstone-capture branch October 31, 2023 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alter the AppExitInfoService to encode NDK protobuf info in escaped chars #22

Alter the AppExitInfoService to encode NDK protobuf info in escaped chars #22

$@fractalwrench$ fractalwrench commented Oct 23, 2023

fnewberg Oct 24, 2023

noonat Oct 24, 2023 •

edited

fnewberg Oct 24, 2023

noonat Oct 24, 2023

noonat Oct 24, 2023

bidetofevil Oct 25, 2023

noonat Oct 25, 2023 •

edited

bidetofevil Oct 25, 2023

noonat Oct 25, 2023

fractalwrench commented Oct 25, 2023

bidetofevil left a comment

bidetofevil Oct 25, 2023

$@fractalwrench$ fractalwrench Oct 25, 2023

bidetofevil Oct 25, 2023

bidetofevil Oct 25, 2023

noonat commented Oct 25, 2023

fractalwrench commented Oct 25, 2023

fnewberg commented Oct 25, 2023

fractalwrench commented Oct 27, 2023

codecov bot commented Oct 27, 2023

bidetofevil left a comment

Alter the AppExitInfoService to encode NDK protobuf info in escaped chars #22

Alter the AppExitInfoService to encode NDK protobuf info in escaped chars #22

Conversation

fractalwrench commented Oct 23, 2023

Goal

Testing

Choose a reason for hiding this comment

noonat Oct 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noonat Oct 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fractalwrench commented Oct 25, 2023

bidetofevil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noonat commented Oct 25, 2023

fractalwrench commented Oct 25, 2023

fnewberg commented Oct 25, 2023

fractalwrench commented Oct 27, 2023

codecov bot commented Oct 27, 2023

Codecov Report

bidetofevil left a comment

Choose a reason for hiding this comment

$@fractalwrench$ fractalwrench commented Oct 23, 2023

noonat Oct 24, 2023 •

edited

noonat Oct 25, 2023 •

edited