Skip to content

Commit ed6b712

Browse files
committed
src: Fix memory leaks in generate_test_instance() by returning values instead of pointers
Problem: The current `generate_test_instance()` function returns `std::list<T*>`, which creates memory management issues: - Inconsistent lifecycle management of test instances - Callers don't always properly clean up allocated memory - Results in ASan memory leak reports in unit tests and ceph-dencoder Solution: Change `generate_test_instance()` to return `std::list<T>` instead of `std::list<T*>`: Core Changes: - Modified all classes with `generate_test_instance()` to return `std::list<T>` - Use `emplace_back()` without parameters** to avoid copy/move constructors for classes that don't define them - Updated ceph-dencoder to handle the new return type ceph-dencoder Adaptations: Since `m_list` now holds `T` objects instead of `T*`, and we can't assume `T` is copyable/moveable: - Keep `m_object` as a pointer for flexibility - Handle two scenarios: 1. `m_object` points to an element in `m_list` 2. `m_object` points to a decoded instance (requires manual cleanup) - Introduce `make_ptr()` as a factory function to create a smart pointer to conditionally free the managed pointer. Additional Cleanup: - Simplify DencoderBase constructor from template to plain function (extra parameters were never used in derived classes) With this change, object lifecycles are now managed by value semantics instead of raw pointers, eliminating memory leaks. Signed-off-by: Kefu Chai <[email protected]>
1 parent 088180d commit ed6b712

File tree

273 files changed

+5217
-4159
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

273 files changed

+5217
-4159
lines changed

src/auth/Auth.h

Lines changed: 37 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,10 @@ struct EntityAuth {
7474
encode_json("caps", caps, f);
7575
f->dump_object("pending_key", pending_key);
7676
}
77-
static void generate_test_instances(std::list<EntityAuth*>& ls) {
78-
ls.push_back(new EntityAuth);
77+
static std::list<EntityAuth> generate_test_instances() {
78+
std::list<EntityAuth> ls;
79+
ls.push_back(EntityAuth{});
80+
return ls;
7981
}
8082
};
8183
WRITE_CLASS_ENCODER(EntityAuth)
@@ -118,13 +120,15 @@ struct AuthCapsInfo {
118120
encode_json("caps", caps, f);
119121
f->dump_unsigned("caps_len", caps.length());
120122
}
121-
static void generate_test_instances(std::list<AuthCapsInfo*>& ls) {
122-
ls.push_back(new AuthCapsInfo);
123-
ls.push_back(new AuthCapsInfo);
124-
ls.back()->allow_all = true;
125-
ls.push_back(new AuthCapsInfo);
126-
ls.back()->caps.append("foo");
127-
ls.back()->caps.append("bar");
123+
static std::list<AuthCapsInfo> generate_test_instances() {
124+
std::list<AuthCapsInfo> ls;
125+
ls.push_back(AuthCapsInfo{});
126+
ls.push_back(AuthCapsInfo{});
127+
ls.back().allow_all = true;
128+
ls.push_back(AuthCapsInfo{});
129+
ls.back().caps.append("foo");
130+
ls.back().caps.append("bar");
131+
return ls;
128132
}
129133
};
130134
WRITE_CLASS_ENCODER(AuthCapsInfo)
@@ -184,15 +188,17 @@ struct AuthTicket {
184188
f->dump_object("caps", caps);
185189
f->dump_unsigned("flags", flags);
186190
}
187-
static void generate_test_instances(std::list<AuthTicket*>& ls) {
188-
ls.push_back(new AuthTicket);
189-
ls.push_back(new AuthTicket);
190-
ls.back()->name.set_id("client.123");
191-
ls.back()->global_id = 123;
192-
ls.back()->init_timestamps(utime_t(123, 456), 7);
193-
ls.back()->caps.caps.append("foo");
194-
ls.back()->caps.caps.append("bar");
195-
ls.back()->flags = 0x12345678;
191+
static std::list<AuthTicket> generate_test_instances() {
192+
std::list<AuthTicket> ls;
193+
ls.push_back(AuthTicket{});
194+
ls.push_back(AuthTicket{});
195+
ls.back().name.set_id("client.123");
196+
ls.back().global_id = 123;
197+
ls.back().init_timestamps(utime_t(123, 456), 7);
198+
ls.back().caps.caps.append("foo");
199+
ls.back().caps.caps.append("bar");
200+
ls.back().flags = 0x12345678;
201+
return ls;
196202
}
197203
};
198204
WRITE_CLASS_ENCODER(AuthTicket)
@@ -282,11 +288,13 @@ struct ExpiringCryptoKey {
282288
f->dump_object("key", key);
283289
f->dump_stream("expiration") << expiration;
284290
}
285-
static void generate_test_instances(std::list<ExpiringCryptoKey*>& ls) {
286-
ls.push_back(new ExpiringCryptoKey);
287-
ls.push_back(new ExpiringCryptoKey);
288-
ls.back()->key.set_secret(
291+
static std::list<ExpiringCryptoKey> generate_test_instances() {
292+
std::list<ExpiringCryptoKey> ls;
293+
ls.push_back(ExpiringCryptoKey{});
294+
ls.push_back(ExpiringCryptoKey{});
295+
ls.back().key.set_secret(
289296
CEPH_CRYPTO_AES, bufferptr("1234567890123456", 16), utime_t(123, 456));
297+
return ls;
290298
}
291299
};
292300
WRITE_CLASS_ENCODER(ExpiringCryptoKey)
@@ -355,11 +363,13 @@ struct RotatingSecrets {
355363
void dump(ceph::Formatter *f) const {
356364
encode_json("secrets", secrets, f);
357365
}
358-
static void generate_test_instances(std::list<RotatingSecrets*>& ls) {
359-
ls.push_back(new RotatingSecrets);
360-
ls.push_back(new RotatingSecrets);
361-
auto eck = new ExpiringCryptoKey;
362-
ls.back()->add(*eck);
366+
static std::list<RotatingSecrets> generate_test_instances() {
367+
std::list<RotatingSecrets> ls;
368+
ls.push_back(RotatingSecrets{});
369+
ls.push_back(RotatingSecrets{});
370+
ExpiringCryptoKey eck{};
371+
ls.back().add(eck);
372+
return ls;
363373
}
364374
};
365375
WRITE_CLASS_ENCODER(RotatingSecrets)

src/auth/Crypto.cc

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -519,14 +519,16 @@ void CryptoKey::dump(Formatter *f) const
519519
f->dump_int("secret.length", secret.length());
520520
}
521521

522-
void CryptoKey::generate_test_instances(std::list<CryptoKey*>& ls)
522+
std::list<CryptoKey> CryptoKey::generate_test_instances()
523523
{
524-
ls.push_back(new CryptoKey);
525-
ls.push_back(new CryptoKey);
526-
ls.back()->type = CEPH_CRYPTO_AES;
527-
ls.back()->set_secret(
524+
std::list<CryptoKey> ls;
525+
ls.emplace_back();
526+
ls.emplace_back();
527+
ls.back().type = CEPH_CRYPTO_AES;
528+
ls.back().set_secret(
528529
CEPH_CRYPTO_AES, bufferptr("1234567890123456", 16), utime_t(123, 456));
529-
ls.back()->created = utime_t(123, 456);
530+
ls.back().created = utime_t(123, 456);
531+
return ls;
530532
}
531533

532534
int CryptoKey::set_secret(int type, const bufferptr& s, utime_t c)

src/auth/Crypto.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ class CryptoKey {
112112
void encode(ceph::buffer::list& bl) const;
113113
void decode(ceph::buffer::list::const_iterator& bl);
114114
void dump(ceph::Formatter *f) const;
115-
static void generate_test_instances(std::list<CryptoKey*>& ls);
115+
static std::list<CryptoKey> generate_test_instances();
116116

117117
void clear() {
118118
*this = CryptoKey();

src/auth/cephx/CephxKeyServer.cc

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -263,9 +263,11 @@ void KeyServer::dump(Formatter *f) const
263263
f->dump_object("data", data);
264264
}
265265

266-
void KeyServer::generate_test_instances(std::list<KeyServer*>& ls)
266+
std::list<KeyServer> KeyServer::generate_test_instances()
267267
{
268-
ls.push_back(new KeyServer(nullptr, nullptr));
268+
std::list<KeyServer> ls;
269+
ls.emplace_back(nullptr, nullptr);
270+
return ls;
269271
}
270272

271273
bool KeyServer::generate_secret(CryptoKey& secret)

src/auth/cephx/CephxKeyServer.h

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,12 @@ struct KeyServerData {
7878
encode_json("secrets", secrets, f);
7979
encode_json("rotating_secrets", rotating_secrets, f);
8080
}
81-
static void generate_test_instances(std::list<KeyServerData*>& ls) {
82-
ls.push_back(new KeyServerData);
83-
ls.push_back(new KeyServerData);
84-
ls.back()->version = 1;
81+
static std::list<KeyServerData> generate_test_instances() {
82+
std::list<KeyServerData> ls;
83+
ls.push_back(KeyServerData{});
84+
ls.push_back(KeyServerData{});
85+
ls.back().version = 1;
86+
return ls;
8587
}
8688
bool contains(const EntityName& name) const {
8789
return (secrets.find(name) != secrets.end());
@@ -176,13 +178,15 @@ struct KeyServerData {
176178
f->dump_object("name", name);
177179
f->dump_object("auth", auth);
178180
}
179-
static void generate_test_instances(std::list<Incremental*>& ls) {
180-
ls.push_back(new Incremental);
181-
ls.back()->op = AUTH_INC_DEL;
182-
ls.push_back(new Incremental);
183-
ls.back()->op = AUTH_INC_ADD;
184-
ls.push_back(new Incremental);
185-
ls.back()->op = AUTH_INC_SET_ROTATING;
181+
static std::list<Incremental> generate_test_instances() {
182+
std::list<Incremental> ls;
183+
ls.push_back(Incremental{});
184+
ls.back().op = AUTH_INC_DEL;
185+
ls.push_back(Incremental{});
186+
ls.back().op = AUTH_INC_ADD;
187+
ls.push_back(Incremental{});
188+
ls.back().op = AUTH_INC_SET_ROTATING;
189+
return ls;
186190
}
187191
};
188192

@@ -274,7 +278,7 @@ class KeyServer : public KeyStore {
274278
decode(data, bl);
275279
}
276280
void dump(ceph::Formatter *f) const;
277-
static void generate_test_instances(std::list<KeyServer*>& ls);
281+
static std::list<KeyServer> generate_test_instances();
278282
bool contains(const EntityName& name) const;
279283
int encode_secrets(ceph::Formatter *f, std::stringstream *ds) const;
280284
void encode_formatted(std::string label, ceph::Formatter *f, ceph::buffer::list &bl);

0 commit comments

Comments
 (0)