-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[scudo] Add __scudo_get_info
symbol to export stats to a buffer.
#130273
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,23 @@ __attribute__((weak)) void __scudo_realloc_deallocate_hook(void *old_ptr); | |
|
||
void __scudo_print_stats(void); | ||
|
||
// Reports all allocators configuration and general statistics as a null | ||
// terminated text string. | ||
#ifndef M_INFO_TOPIC_STATS | ||
#define M_INFO_TOPIC_STATS 1 | ||
#endif | ||
|
||
// Reports fragmentation statistics of the primary allocation as a null | ||
// terminated text string. | ||
#ifndef M_INFO_TOPIC_FRAGMENTATION | ||
#define M_INFO_TOPIC_FRAGMENTATION 2 | ||
#endif | ||
|
||
// Writes allocator statistics to the buffer, truncating to the specified size | ||
// if necessary. Returns the full report size (before truncation) for buffer | ||
// sizing purpose, or zero if the topic is not supported. | ||
size_t __scudo_get_info(uint32_t topic, void *buffer, size_t size); | ||
piwicode marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought I had a comment about this, but I may have missed your reply. Is there a reason to have this as one function and not two different functions that do different things? For example, are you planning to add more info types in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I want to allow for future topic additions without altering the binary interface. This approach enables us to experiment with new topics and validate their usefulness before formally documenting them. It also provides a graceful way to deprecate topics without breaking existing callers or cluttering the symbol table. |
||
|
||
typedef void (*iterate_callback)(uintptr_t base, size_t size, void *arg); | ||
|
||
// Determine the likely cause of a tag check fault or other memory protection | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -229,6 +229,16 @@ void ScopedString::append(const char *Format, ...) { | |
va_end(Args); | ||
} | ||
|
||
size_t ScopedString::copyToBuffer(char *OutputBase, size_t OutputLength) { | ||
DCHECK(OutputBase); | ||
if (OutputLength) { | ||
const size_t Written = Min(length(), OutputLength - 1); | ||
memcpy(OutputBase, data(), Written); | ||
OutputBase[Written] = '\0'; | ||
} | ||
return length() + 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should return the length of data copied? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I tried to conform to existing APIs, they are a bit surprising, but it makes sense. In combined.h, getStats uses that protocol, where the caller can provide a buffer, and if it is too small, the value gets truncated, and the caller knows it from the returned size. I reuse the same principle here. |
||
} | ||
|
||
void Printf(const char *Format, ...) { | ||
va_list Args; | ||
va_start(Args, Format); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -333,6 +333,12 @@ void ScudoCombinedTest<Config>::BasicTest(scudo::uptr SizeLog) { | |
|
||
Allocator->printStats(); | ||
Allocator->printFragmentationInfo(); | ||
|
||
{ | ||
char buffer[256] = {0}; | ||
EXPECT_LE(0, Allocator->getStats(buffer, sizeof(buffer))); | ||
EXPECT_LE(0, Allocator->getFragmentationInfo(buffer, sizeof(buffer))); | ||
Comment on lines
+339
to
+340
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change these to EXPECT_GT(Allocator->..., 0); Same as the uses in the tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
} | ||
|
||
#define SCUDO_MAKE_BASIC_TEST(SizeLog) \ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,49 @@ TEST(ScudoStringsTest, Padding) { | |
testAgainstLibc<int>("%03d - %03d", -12, -1234); | ||
} | ||
|
||
TEST(ScudoStringsTest, CopyFromAnEmptyString) { | ||
scudo::ScopedString Str; | ||
char buf[256] = {'0', '1'}; | ||
EXPECT_EQ(1U, Str.copyToBuffer(buf, sizeof(buf))); | ||
EXPECT_STREQ("", buf); | ||
EXPECT_EQ(0, buf[0]); // Rest of the buffer remains unchanged. | ||
} | ||
|
||
TEST(ScudoStringsTest, CopyFromAnEmptyStringIntoZeroSizeBuffer) { | ||
scudo::ScopedString Str; | ||
char buf[256] = {'0', '1'}; | ||
EXPECT_EQ(1U, Str.copyToBuffer(buf, 0)); | ||
EXPECT_EQ('0', buf[0]); // Nothing changed because provided size is 0. | ||
} | ||
|
||
TEST(ScudoStringsTest, CopyIntoLargeEnoughBuffer) { | ||
scudo::ScopedString Str; | ||
Str.append("abc"); | ||
char buf[256] = {'0', '1', '2', '3', '4', '5'}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me that we don't need to init the buf with string nubmers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to check for overflow. Fixed. |
||
// Size includes terminal null. | ||
EXPECT_EQ(4U, Str.copyToBuffer(buf, sizeof(buf))); | ||
EXPECT_STREQ("abc", buf); | ||
EXPECT_EQ(buf[4], '4'); | ||
} | ||
|
||
TEST(ScudoStringsTest, CopyWithTextOverflow) { | ||
scudo::ScopedString Str; | ||
Str.append("abc"); | ||
char buf[256] = {'0', '1', '2', '3', '4', '5'}; | ||
EXPECT_EQ(4U, Str.copyToBuffer(buf, 3)); | ||
EXPECT_STREQ("ab", buf); | ||
EXPECT_EQ(buf[3], '3'); | ||
} | ||
|
||
TEST(ScudoStringsTest, CopyIntoExactFit) { | ||
scudo::ScopedString Str; | ||
Str.append("abc"); | ||
char buf[256] = {'0', '1', '2', '3', '4', '5'}; | ||
EXPECT_EQ(4U, Str.copyToBuffer(buf, 4)); | ||
EXPECT_STREQ("abc", buf); | ||
EXPECT_EQ(buf[4], '4'); | ||
} | ||
|
||
#if defined(__linux__) | ||
|
||
#include <sys/resource.h> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
#include "internal_defs.h" | ||
#include "platform.h" | ||
#include "scudo/interface.h" | ||
#include "string_utils.h" | ||
#include "wrappers_c.h" | ||
#include "wrappers_c_checks.h" | ||
|
||
|
@@ -37,4 +38,19 @@ scudo::Allocator<scudo::Config, SCUDO_PREFIX(malloc_postinit)> SCUDO_ALLOCATOR; | |
|
||
extern "C" INTERFACE void __scudo_print_stats(void) { Allocator.printStats(); } | ||
|
||
extern "C" INTERFACE size_t __scudo_get_info(uint32_t topic, void *buffer, | ||
size_t size) { | ||
switch (topic) { | ||
case M_INFO_TOPIC_STATS: | ||
return Allocator.getStats(reinterpret_cast<char *>(buffer), size); | ||
case M_INFO_TOPIC_FRAGMENTATION: | ||
return Allocator.getFragmentationInfo(reinterpret_cast<char *>(buffer), | ||
size); | ||
default: | ||
scudo::Printf("Scudo WARNING: unrecognized __scudo_get_info topic: %d\n", | ||
topic); | ||
return 0; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you want a default case here and below? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can rewrite with a default case. Is it better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think it's better to warn any invalid options than silent it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Now it prints a warning message. |
||
} | ||
|
||
#endif // !SCUDO_ANDROID || !_BIONIC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All parameters are capitalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like parameters use snake_case in the interface.h. Could you please confirm that we want to Capitalize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, we have so many different cases. We should probably change this in the future, but your interface is fine.