Skip to content
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.

Sign up for GitHub

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

C APIのセーフティネットのメッセージを改善する #625

Merged
merged 6 commits into from
Oct 8, 2023
Merged

C APIのセーフティネットのメッセージを改善する #625

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2c9f644
C APIのセーフティネットのメッセージを改善する
qryxip Sep 29, 2023
2561fda
メッセージを更新
qryxip Sep 29, 2023
b27f1b0
`assert!(p, "…")` → `if !p { panic!("…"); }`
qryxip Sep 30, 2023
b4a8541
Merge branch 'main' into improve-panic-messages-for-duplicated-ptrs
qryxip Oct 1, 2023
5718353
メッセージを更新
qryxip Oct 1, 2023
080b2be
`drop_check`の方にテストを入れる
qryxip Oct 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions crates/voicevox_core_c_api/src/drop_check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,15 @@ impl CStringDropChecker {
owned_str_addrs, ..
} = &mut *self.0.lock().unwrap();

let duplicated = !owned_str_addrs.insert(s.as_ptr() as usize);
assert!(!duplicated, "duplicated");
let ptr = s.as_ptr();
let duplicated = !owned_str_addrs.insert(ptr as usize);
if duplicated {
panic!(
"別の{ptr:p}が管理下にあります。原因としては以前に別の文字列が{ptr:p}として存在\
しており、それが誤った形で解放されたことが考えられます。このライブラリで生成した\
オブジェクトの解放は、このライブラリが提供するAPIで行われなくてはなりません",
);
}
Comment on lines +49 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

意図的に重複を引き当てるケースを作ることが難しいことに気付きました。

たぶんダメなんだろうなと思いつつのコメントなのですが、同じ変数を２回指定するのはだめですか。

let s: CString = "hoge"; // これで動くかわからないのですが、適当なCStringを定義したいくらいの意図です
whitelist(s);
whitelist(s);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slice_ownerの方は厳しいですが、drop_checkの方だと(例示して下さったコードのままだと無理ですが)確かにいけますね!

080b2be (#625)

Copy link
Member Author

@qryxip qryxip Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slice_ownerの方も、Allocatorが安定化(stabilized)してくれればそれっぽいアロケータを作ってVecの第2型引数に入れればいけるんですけどね。今のstable Rustで挿げ替えることができるのはGlobalAllocだけなので、例えプロセスを分離した上であってもSliceOwnerの中で使っているBTreeMapが死んでしまう... (それ以前にtest harness自体とかも死ぬ可能性すらある)

s
}

Expand Down
8 changes: 7 additions & 1 deletion crates/voicevox_core_c_api/src/slice_owner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,13 @@ impl<T> SliceOwner<T> {
let len = slice.len();

let duplicated = slices.insert(ptr as usize, slice.into()).is_some();
assert!(!duplicated, "duplicated");
if duplicated {
panic!(
"別の{ptr:p}が管理下にあります。原因としては以前に別の配列が{ptr:p}として存在\
しており、それが誤った形で解放されたことが考えられます。このライブラリで生成した\
オブジェクトの解放は、このライブラリが提供するAPIで行われなくてはなりません",
);
}

out_ptr.as_ptr().write_unaligned(ptr);
out_len.as_ptr().write_unaligned(len);
Expand Down
Loading