Skip to content

Commit 1ebe98f

Browse files
xdustinfacexdustinface
committed
fix(ffi): Prevent FFIError message leaks in tests
- Modified set_error(), set_success(), and ffi_error_set! macro to free previous error messages before setting new ones - Added FFIError::free_message() helper for test cleanup - Added cleanup calls to mnemonic_tests.rs, address_tests.rs, wallet_tests.rs, and wallet_manager_tests.rs tests
1 parent 777b84f commit 1ebe98f

5 files changed

Lines changed: 134 additions & 11 deletions

File tree

‎key-wallet-ffi/src/address_tests.rs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ mod address_tests {
2929
let is_valid = unsafe { address_validate(ptr::null(), FFINetwork::Testnet, error) };
3030
assert!(!is_valid);
3131
assert_eq!(unsafe { (*error).code }, FFIErrorCode::InvalidInput);
32+
33+
unsafe { (*error).free_message() };
3234
}
3335

3436
#[test]
@@ -43,6 +45,8 @@ mod address_tests {
4345
assert_eq!(unsafe { (*error).code }, FFIErrorCode::Success);
4446
// Returns 0 for P2PKH
4547
assert_eq!(addr_type, 0);
48+
49+
unsafe { (*error).free_message() };
4650
}
4751

4852
#[test]
@@ -55,6 +59,8 @@ mod address_tests {
5559
unsafe { address_validate(addr_str.as_ptr(), FFINetwork::Testnet, &mut error) };
5660

5761
assert!(is_valid);
62+
63+
unsafe { error.free_message() };
5864
}
5965

6066
#[test]
@@ -68,6 +74,8 @@ mod address_tests {
6874

6975
assert!(!is_valid);
7076
assert_eq!(error.code, FFIErrorCode::InvalidAddress);
77+
78+
unsafe { error.free_message() };
7179
}
7280

7381
#[test]
@@ -78,6 +86,8 @@ mod address_tests {
7886

7987
assert!(!is_valid);
8088
assert_eq!(error.code, FFIErrorCode::InvalidInput);
89+
90+
unsafe { error.free_message() };
8191
}
8292

8393
#[test]
@@ -97,6 +107,8 @@ mod address_tests {
97107
assert!(addr_type <= 2);
98108
assert_eq!(error.code, FFIErrorCode::Success);
99109
}
110+
111+
unsafe { error.free_message() };
100112
}
101113

102114
#[test]
@@ -110,6 +122,8 @@ mod address_tests {
110122
// Should return 255 (u8::MAX) for invalid
111123
assert_eq!(addr_type, 255);
112124
assert_eq!(error.code, FFIErrorCode::InvalidAddress);
125+
126+
unsafe { error.free_message() };
113127
}
114128

115129
#[test]
@@ -121,6 +135,8 @@ mod address_tests {
121135
// Should return 255 (u8::MAX) for null input
122136
assert_eq!(addr_type, 255);
123137
assert_eq!(error.code, FFIErrorCode::InvalidInput);
138+
139+
unsafe { error.free_message() };
124140
}
125141

126142
#[test]
@@ -177,6 +193,8 @@ mod address_tests {
177193
let is_valid = address_validate(addr_str.as_ptr(), FFINetwork::Testnet, &mut error);
178194
assert!(!is_valid);
179195
}
196+
197+
error.free_message();
180198
}
181199
}
182200

@@ -200,6 +218,8 @@ mod address_tests {
200218
// Should return a valid type (0, 1, 2) or 255 for error
201219
assert!(addr_type <= 2 || addr_type == 255);
202220
}
221+
222+
error.free_message();
203223
}
204224
}
205225
}

‎key-wallet-ffi/src/error.rs‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,25 +48,50 @@ impl FFIError {
4848
}
4949
}
5050

51-
/// Set error on a mutable pointer if it's not null
51+
/// Set error on a mutable pointer if it's not null.
52+
/// Frees any previous error message before setting the new one.
5253
#[allow(clippy::not_unsafe_ptr_arg_deref)]
5354
pub fn set_error(error_ptr: *mut FFIError, code: FFIErrorCode, msg: String) {
5455
if !error_ptr.is_null() {
5556
unsafe {
57+
// Free previous message if present
58+
let prev = &mut *error_ptr;
59+
if !prev.message.is_null() {
60+
let _ = CString::from_raw(prev.message);
61+
}
5662
*error_ptr = Self::error(code, msg);
5763
}
5864
}
5965
}
6066

61-
/// Set success on a mutable pointer if it's not null
67+
/// Set success on a mutable pointer if it's not null.
68+
/// Frees any previous error message before setting success.
6269
#[allow(clippy::not_unsafe_ptr_arg_deref)]
6370
pub fn set_success(error_ptr: *mut FFIError) {
6471
if !error_ptr.is_null() {
6572
unsafe {
73+
// Free previous message if present
74+
let prev = &mut *error_ptr;
75+
if !prev.message.is_null() {
76+
let _ = CString::from_raw(prev.message);
77+
}
6678
*error_ptr = Self::success();
6779
}
6880
}
6981
}
82+
83+
/// Free the error message if present.
84+
/// Use this in tests to prevent memory leaks.
85+
///
86+
/// # Safety
87+
///
88+
/// The message pointer must have been allocated by this library.
89+
pub unsafe fn free_message(&mut self) {
90+
if !self.message.is_null() {
91+
let _ = CString::from_raw(self.message);
92+
self.message = ptr::null_mut();
93+
}
94+
}
7095
}
7196

7297
/// Free an error message
@@ -83,13 +108,19 @@ pub unsafe extern "C" fn error_message_free(message: *mut c_char) {
83108
}
84109
}
85110

86-
/// Helper macro to convert any error that implements `Into<FFIError>` and set it on the error pointer
111+
/// Helper macro to convert any error that implements `Into<FFIError>` and set it on the error pointer.
112+
/// Frees any previous error message before setting the new one.
87113
#[macro_export]
88114
macro_rules! ffi_error_set {
89115
($error_ptr:expr, $err:expr) => {{
90116
let ffi_error: $crate::error::FFIError = $err.into();
91117
if !$error_ptr.is_null() {
92118
unsafe {
119+
// Free previous message if present
120+
let prev = &mut *$error_ptr;
121+
if !prev.message.is_null() {
122+
let _ = std::ffi::CString::from_raw(prev.message);
123+
}
93124
*$error_ptr = ffi_error;
94125
}
95126
}

‎key-wallet-ffi/src/mnemonic_tests.rs‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ mod tests {
3838
let is_valid = unsafe { mnemonic::mnemonic_validate(ptr::null(), error) };
3939
assert!(!is_valid);
4040
assert_eq!(unsafe { (*error).code }, FFIErrorCode::InvalidInput);
41+
42+
unsafe { (*error).free_message() };
4143
}
4244

4345
#[test]
@@ -78,6 +80,8 @@ mod tests {
7880
let invalid = mnemonic::mnemonic_generate(13, error);
7981
assert!(invalid.is_null());
8082
assert_eq!(unsafe { (*error).code }, FFIErrorCode::InvalidInput);
83+
84+
unsafe { (*error).free_message() };
8185
}
8286

8387
#[test]
@@ -121,6 +125,8 @@ mod tests {
121125

122126
assert!(success);
123127
assert_ne!(seed, seed_with_pass); // Different passphrase should produce different seed
128+
129+
unsafe { (*error).free_message() };
124130
}
125131

126132
#[test]
@@ -143,6 +149,8 @@ mod tests {
143149
mnemonic::mnemonic_free(mnemonic);
144150
}
145151
}
152+
153+
unsafe { (*error).free_message() };
146154
}
147155

148156
#[test]
@@ -158,6 +166,8 @@ mod tests {
158166
assert!(mnemonic.is_null());
159167
assert_eq!(unsafe { (*error).code }, FFIErrorCode::InvalidInput);
160168
}
169+
170+
unsafe { (*error).free_message() };
161171
}
162172

163173
#[test]
@@ -195,6 +205,8 @@ mod tests {
195205
};
196206
assert!(success);
197207
assert_eq!(seed_len, 64);
208+
209+
unsafe { (*error).free_message() };
198210
}
199211

200212
#[test]
@@ -230,6 +242,8 @@ mod tests {
230242
mnemonic::mnemonic_free(mnemonic_ptr);
231243
}
232244
}
245+
246+
unsafe { error.free_message() };
233247
}
234248

235249
#[test]
@@ -269,6 +283,8 @@ mod tests {
269283

270284
mnemonic::mnemonic_free(portuguese_mnemonic);
271285
}
286+
287+
unsafe { error.free_message() };
272288
}
273289

274290
#[test]
@@ -291,6 +307,8 @@ mod tests {
291307
unsafe {
292308
mnemonic::mnemonic_free(mnemonic_ptr);
293309
}
310+
311+
unsafe { error.free_message() };
294312
}
295313

296314
#[test]
@@ -340,6 +358,8 @@ mod tests {
340358

341359
// Seeds should be different
342360
assert_ne!(seed1, seed2);
361+
362+
unsafe { error.free_message() };
343363
}
344364

345365
#[test]
@@ -363,6 +383,8 @@ mod tests {
363383
assert_eq!(count, expected_count);
364384
assert_eq!(error.code, FFIErrorCode::Success);
365385
}
386+
387+
error.free_message();
366388
}
367389
}
368390

@@ -374,6 +396,8 @@ mod tests {
374396

375397
assert_eq!(count, 0);
376398
assert_eq!(error.code, FFIErrorCode::InvalidInput);
399+
400+
unsafe { error.free_message() };
377401
}
378402

379403
#[test]
@@ -391,6 +415,8 @@ mod tests {
391415

392416
assert_eq!(count, 0);
393417
assert_eq!(error.code, FFIErrorCode::InvalidInput);
418+
419+
unsafe { error.free_message() };
394420
}
395421

396422
#[test]
@@ -438,6 +464,8 @@ mod tests {
438464
};
439465
assert!(!success);
440466
assert_eq!(error.code, FFIErrorCode::InvalidInput);
467+
468+
unsafe { error.free_message() };
441469
}
442470

443471
#[test]
@@ -459,6 +487,8 @@ mod tests {
459487

460488
assert!(!success);
461489
assert_eq!(error.code, FFIErrorCode::InvalidMnemonic);
490+
491+
unsafe { error.free_message() };
462492
}
463493

464494
#[test]
@@ -494,6 +524,8 @@ mod tests {
494524
};
495525
assert!(!success);
496526
assert_eq!(error.code, FFIErrorCode::InvalidInput);
527+
528+
unsafe { error.free_message() };
497529
}
498530

499531
#[test]
@@ -511,6 +543,8 @@ mod tests {
511543

512544
assert!(!is_valid);
513545
assert_eq!(error.code, FFIErrorCode::InvalidInput);
546+
547+
unsafe { error.free_message() };
514548
}
515549

516550
#[test]
@@ -526,6 +560,8 @@ mod tests {
526560

527561
assert!(mnemonic.is_null());
528562
assert_eq!(error.code, FFIErrorCode::InvalidInput);
563+
564+
unsafe { error.free_message() };
529565
}
530566

531567
#[test]
@@ -552,6 +588,8 @@ mod tests {
552588
mnemonic::mnemonic_free(mnemonic);
553589
}
554590
}
591+
592+
unsafe { error.free_message() };
555593
}
556594

557595
#[test]
@@ -589,6 +627,8 @@ mod tests {
589627
}
590628
}
591629
}
630+
631+
unsafe { error.free_message() };
592632
}
593633

594634
#[test]
@@ -634,6 +674,7 @@ mod tests {
634674

635675
unsafe {
636676
mnemonic::mnemonic_free(mnemonic);
677+
error.free_message();
637678
}
638679
}
639680

@@ -679,6 +720,7 @@ mod tests {
679720
// Free
680721
unsafe {
681722
mnemonic::mnemonic_free(mnemonic);
723+
error.free_message();
682724
}
683725
}
684726
}

0 commit comments

Comments
 (0)