diff --git a/libstats/push_compat/StatsEventCompat.cpp b/libstats/push_compat/StatsEventCompat.cpp index c17ca61f1..12a5dd4a7 100644 --- a/libstats/push_compat/StatsEventCompat.cpp +++ b/libstats/push_compat/StatsEventCompat.cpp @@ -224,7 +224,6 @@ void StatsEventCompat::addInt32Annotation(uint8_t annotationId, int32_t value) { int StatsEventCompat::writeToSocket() { if (useRSchema()) { - mAStatsEventApi.build(mEventR); return mAStatsEventApi.write(mEventR); } diff --git a/libstats/socket/include/stats_event.h b/libstats/socket/include/stats_event.h index 00dc76e3d..601a1815a 100644 --- a/libstats/socket/include/stats_event.h +++ b/libstats/socket/include/stats_event.h @@ -35,7 +35,6 @@ * AStatsEvent_addInt32Annotation(event, 2, 128); * AStatsEvent_writeFloat(event, 2.0); * - * AStatsEvent_build(event); * AStatsEvent_write(event); * AStatsEvent_release(event); * @@ -61,10 +60,11 @@ typedef struct AStatsEvent AStatsEvent; AStatsEvent* AStatsEvent_obtain(); /** - * Builds and finalizes the StatsEvent. + * Builds and finalizes the AStatsEvent for a pulled event. + * This should only be called for pulled AStatsEvents. * * After this function, the StatsEvent must not be modified in any way other than calling release or - * write. Build must be always be called before AStatsEvent_write. + * write. * * Build can be called multiple times without error. * If the event has been built before, this function is a no-op. diff --git a/libstats/socket/stats_event.c b/libstats/socket/stats_event.c index e63bc07db..a94b3a1ed 100644 --- a/libstats/socket/stats_event.c +++ b/libstats/socket/stats_event.c @@ -23,7 +23,9 @@ #define LOGGER_ENTRY_MAX_PAYLOAD 4068 // Max payload size is 4 bytes less as 4 bytes are reserved for stats_eventTag. // See android_util_Stats_Log.cpp -#define MAX_EVENT_PAYLOAD (LOGGER_ENTRY_MAX_PAYLOAD - 4) +#define MAX_PUSH_EVENT_PAYLOAD (LOGGER_ENTRY_MAX_PAYLOAD - 4) + +#define MAX_PULL_EVENT_PAYLOAD (50 * 1024) // 50 KB /* POSITIONS */ #define POS_NUM_ELEMENTS 1 @@ -70,12 +72,13 @@ struct AStatsEvent { // metadata field (e.g. timestamp) or an atom field. size_t lastFieldPos; // Number of valid bytes within the buffer. - size_t size; + size_t numBytesWritten; uint32_t numElements; uint32_t atomId; uint32_t errors; bool truncate; bool built; + size_t bufSize; }; static int64_t get_elapsed_realtime_ns() { @@ -87,14 +90,15 @@ static int64_t get_elapsed_realtime_ns() { AStatsEvent* AStatsEvent_obtain() { AStatsEvent* event = malloc(sizeof(AStatsEvent)); - event->buf = (uint8_t*)calloc(MAX_EVENT_PAYLOAD, 1); event->lastFieldPos = 0; - event->size = 2; // reserve first two bytes for outer event type and number of elements + event->numBytesWritten = 2; // reserve first 2 bytes for root event type and number of elements event->numElements = 0; event->atomId = 0; event->errors = 0; event->truncate = true; // truncate for both pulled and pushed atoms event->built = false; + event->bufSize = MAX_PUSH_EVENT_PAYLOAD; + event->buf = (uint8_t*)calloc(event->bufSize, 1); event->buf[0] = OBJECT_TYPE; AStatsEvent_writeInt64(event, get_elapsed_realtime_ns()); // write the timestamp @@ -128,19 +132,33 @@ void AStatsEvent_overwriteTimestamp(AStatsEvent* event, uint64_t timestampNs) { // Side-effect: modifies event->errors if the buffer would overflow static bool overflows(AStatsEvent* event, size_t size) { - if (event->size + size > MAX_EVENT_PAYLOAD) { + const size_t totalBytesNeeded = event->numBytesWritten + size; + if (totalBytesNeeded > MAX_PULL_EVENT_PAYLOAD) { event->errors |= ERROR_OVERFLOW; return true; } + + // Expand buffer if needed. + if (event->bufSize < MAX_PULL_EVENT_PAYLOAD && totalBytesNeeded > event->bufSize) { + do { + event->bufSize *= 2; + } while (event->bufSize <= totalBytesNeeded); + + if (event->bufSize > MAX_PULL_EVENT_PAYLOAD) { + event->bufSize = MAX_PULL_EVENT_PAYLOAD; + } + + event->buf = (uint8_t*)realloc(event->buf, event->bufSize); + } return false; } -// Side-effect: all append functions increment event->size if there is +// Side-effect: all append functions increment event->numBytesWritten if there is // sufficient space within the buffer to place the value static void append_byte(AStatsEvent* event, uint8_t value) { if (!overflows(event, sizeof(value))) { - event->buf[event->size] = value; - event->size += sizeof(value); + event->buf[event->numBytesWritten] = value; + event->numBytesWritten += sizeof(value); } } @@ -150,36 +168,36 @@ static void append_bool(AStatsEvent* event, bool value) { static void append_int32(AStatsEvent* event, int32_t value) { if (!overflows(event, sizeof(value))) { - memcpy(&event->buf[event->size], &value, sizeof(value)); - event->size += sizeof(value); + memcpy(&event->buf[event->numBytesWritten], &value, sizeof(value)); + event->numBytesWritten += sizeof(value); } } static void append_int64(AStatsEvent* event, int64_t value) { if (!overflows(event, sizeof(value))) { - memcpy(&event->buf[event->size], &value, sizeof(value)); - event->size += sizeof(value); + memcpy(&event->buf[event->numBytesWritten], &value, sizeof(value)); + event->numBytesWritten += sizeof(value); } } static void append_float(AStatsEvent* event, float value) { if (!overflows(event, sizeof(value))) { - memcpy(&event->buf[event->size], &value, sizeof(value)); - event->size += sizeof(float); + memcpy(&event->buf[event->numBytesWritten], &value, sizeof(value)); + event->numBytesWritten += sizeof(float); } } static void append_byte_array(AStatsEvent* event, const uint8_t* buf, size_t size) { if (!overflows(event, size)) { - memcpy(&event->buf[event->size], buf, size); - event->size += size; + memcpy(&event->buf[event->numBytesWritten], buf, size); + event->numBytesWritten += size; } } // Side-effect: modifies event->errors if buf is not properly null-terminated static void append_string(AStatsEvent* event, const char* buf) { - size_t size = strnlen(buf, MAX_EVENT_PAYLOAD); - if (size == MAX_EVENT_PAYLOAD) { + size_t size = strnlen(buf, MAX_PULL_EVENT_PAYLOAD); + if (size == MAX_PULL_EVENT_PAYLOAD) { event->errors |= ERROR_STRING_NOT_NULL_TERMINATED; return; } @@ -189,7 +207,7 @@ static void append_string(AStatsEvent* event, const char* buf) { } static void start_field(AStatsEvent* event, uint8_t typeId) { - event->lastFieldPos = event->size; + event->lastFieldPos = event->numBytesWritten; append_byte(event, typeId); event->numElements++; } @@ -292,7 +310,7 @@ uint32_t AStatsEvent_getAtomId(AStatsEvent* event) { } uint8_t* AStatsEvent_getBuffer(AStatsEvent* event, size_t* size) { - if (size) *size = event->size; + if (size) *size = event->numBytesWritten; return event->buf; } @@ -304,10 +322,10 @@ void AStatsEvent_truncateBuffer(AStatsEvent* event, bool truncate) { event->truncate = truncate; } -void AStatsEvent_build(AStatsEvent* event) { - if (event->built) return; - +static void build_internal(AStatsEvent* event, const bool push) { if (event->numElements > MAX_BYTE_VALUE) event->errors |= ERROR_TOO_MANY_FIELDS; + if (0 == event->atomId) event->errors |= ERROR_NO_ATOM_ID; + if (push && event->numBytesWritten > MAX_PUSH_EVENT_PAYLOAD) event->errors |= ERROR_OVERFLOW; // If there are errors, rewrite buffer. if (event->errors) { @@ -317,7 +335,7 @@ void AStatsEvent_build(AStatsEvent* event) { // Reset number of atom-level annotations to 0. event->buf[POS_ATOM_ID] = INT32_TYPE; // Now, write errors to the buffer immediately after the atom id. - event->size = POS_ATOM_ID + sizeof(uint8_t) + sizeof(uint32_t); + event->numBytesWritten = POS_ATOM_ID + sizeof(uint8_t) + sizeof(uint32_t); start_field(event, ERROR_TYPE); append_int32(event, event->errors); } @@ -326,12 +344,21 @@ void AStatsEvent_build(AStatsEvent* event) { // Truncate the buffer to the appropriate length in order to limit our // memory usage. - if (event->truncate) event->buf = (uint8_t*)realloc(event->buf, event->size); + if (event->truncate) { + event->buf = (uint8_t*)realloc(event->buf, event->numBytesWritten); + event->bufSize = event->numBytesWritten; + } +} + +void AStatsEvent_build(AStatsEvent* event) { + if (event->built) return; + + build_internal(event, false /* push */); event->built = true; } int AStatsEvent_write(AStatsEvent* event) { - AStatsEvent_build(event); - return write_buffer_to_statsd(event->buf, event->size, event->atomId); + build_internal(event, true /* push */); + return write_buffer_to_statsd(event->buf, event->numBytesWritten, event->atomId); } diff --git a/libstats/socket/tests/stats_event_test.cpp b/libstats/socket/tests/stats_event_test.cpp index 80ef145ae..cc2552119 100644 --- a/libstats/socket/tests/stats_event_test.cpp +++ b/libstats/socket/tests/stats_event_test.cpp @@ -343,27 +343,91 @@ TEST(StatsEventTest, TestNoAtomIdError) { AStatsEvent_build(event); uint32_t errors = AStatsEvent_getErrors(event); - EXPECT_NE(errors | ERROR_NO_ATOM_ID, 0); + EXPECT_EQ(errors & ERROR_NO_ATOM_ID, ERROR_NO_ATOM_ID); AStatsEvent_release(event); } -TEST(StatsEventTest, TestOverflowError) { +TEST(StatsEventTest, TestPushOverflowError) { + const char* str = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const int writeCount = 120; // Number of times to write str in the event. + AStatsEvent* event = AStatsEvent_obtain(); AStatsEvent_setAtomId(event, 100); - // Add 1000 int32s to the event. Each int32 takes 5 bytes so this will + + // Add str to the event 120 times. Each str takes >35 bytes so this will // overflow the 4068 byte buffer. - for (int i = 0; i < 1000; i++) { - AStatsEvent_writeInt32(event, 0); + // We want to keep writeCount less than 127 to avoid hitting + // ERROR_TOO_MANY_FIELDS. + for (int i = 0; i < writeCount; i++) { + AStatsEvent_writeString(event, str); + } + AStatsEvent_write(event); + + uint32_t errors = AStatsEvent_getErrors(event); + EXPECT_EQ(errors & ERROR_OVERFLOW, ERROR_OVERFLOW); + + AStatsEvent_release(event); +} + +TEST(StatsEventTest, TestPullOverflowError) { + const uint32_t atomId = 10100; + const vector bytes(430 /* number of elements */, 1 /* value of each element */); + const int writeCount = 120; // Number of times to write bytes in the event. + + AStatsEvent* event = AStatsEvent_obtain(); + AStatsEvent_setAtomId(event, atomId); + + // Add bytes to the event 120 times. Size of bytes is 430 so this will + // overflow the 50 KB pulled event buffer. + // We want to keep writeCount less than 127 to avoid hitting + // ERROR_TOO_MANY_FIELDS. + for (int i = 0; i < writeCount; i++) { + AStatsEvent_writeByteArray(event, bytes.data(), bytes.size()); } AStatsEvent_build(event); uint32_t errors = AStatsEvent_getErrors(event); - EXPECT_NE(errors | ERROR_OVERFLOW, 0); + EXPECT_EQ(errors & ERROR_OVERFLOW, ERROR_OVERFLOW); AStatsEvent_release(event); } +TEST(StatsEventTest, TestLargePull) { + const uint32_t atomId = 100; + const string str = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + const int writeCount = 120; // Number of times to write str in the event. + const int64_t startTime = android::elapsedRealtimeNano(); + + AStatsEvent* event = AStatsEvent_obtain(); + AStatsEvent_setAtomId(event, atomId); + + // Add str to the event 120 times. + // We want to keep writeCount less than 127 to avoid hitting + // ERROR_TOO_MANY_FIELDS. + for (int i = 0; i < writeCount; i++) { + AStatsEvent_writeString(event, str.c_str()); + } + AStatsEvent_build(event); + int64_t endTime = android::elapsedRealtimeNano(); + + size_t bufferSize; + uint8_t* buffer = AStatsEvent_getBuffer(event, &bufferSize); + uint8_t* bufferEnd = buffer + bufferSize; + + checkMetadata(&buffer, writeCount, startTime, endTime, atomId); + + // Check all instances of str have been written. + for (int i = 0; i < writeCount; i++) { + checkTypeHeader(&buffer, STRING_TYPE); + checkString(&buffer, str); + } + + EXPECT_EQ(buffer, bufferEnd); // Ensure that we have read the entire buffer. + EXPECT_EQ(AStatsEvent_getErrors(event), 0); + AStatsEvent_release(event); +} + TEST(StatsEventTest, TestAtomIdInvalidPositionError) { AStatsEvent* event = AStatsEvent_obtain(); AStatsEvent_writeInt32(event, 0); @@ -372,7 +436,7 @@ TEST(StatsEventTest, TestAtomIdInvalidPositionError) { AStatsEvent_build(event); uint32_t errors = AStatsEvent_getErrors(event); - EXPECT_NE(errors | ERROR_ATOM_ID_INVALID_POSITION, 0); + EXPECT_EQ(errors & ERROR_ATOM_ID_INVALID_POSITION, ERROR_ATOM_ID_INVALID_POSITION); AStatsEvent_release(event); } diff --git a/libstats/socket/tests/stats_writer_test.cpp b/libstats/socket/tests/stats_writer_test.cpp index 47f3517b8..749599ff3 100644 --- a/libstats/socket/tests/stats_writer_test.cpp +++ b/libstats/socket/tests/stats_writer_test.cpp @@ -20,12 +20,9 @@ #include "stats_socket.h" TEST(StatsWriterTest, TestSocketClose) { - EXPECT_TRUE(stats_log_is_closed()); - AStatsEvent* event = AStatsEvent_obtain(); AStatsEvent_setAtomId(event, 100); AStatsEvent_writeInt32(event, 5); - AStatsEvent_build(event); int successResult = AStatsEvent_write(event); AStatsEvent_release(event); @@ -36,4 +33,4 @@ TEST(StatsWriterTest, TestSocketClose) { AStatsSocket_close(); EXPECT_TRUE(stats_log_is_closed()); -} \ No newline at end of file +}