-
Notifications
You must be signed in to change notification settings - Fork 120
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
[project-vvm-async-api] "buffer"をRustの世界で保持し続ける #525
Merged
Hiroshiba
merged 13 commits into
VOICEVOX:project-vvm-async-api
from
qryxip:project-vvm-async-api-refactor-buffer-manager
Jun 20, 2023
Merged
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
78f7477
"buffer"をRustの世界で保持し続ける
qryxip 1557e7d
抜けていた、二重登録に対するassertを入れる
qryxip 5e55c54
不要となった`unsafe`を外す
qryxip 263caa9
docstringを書く
qryxip 908ae9c
docを修正
qryxip 44bbe5b
docを更新
qryxip 12f6d35
docを更新
qryxip 28a8c26
docを更新
qryxip 156c2b2
`CStringOwnerInner` → `Inner`
qryxip c1f2a1f
`delete` → `drop_for`
qryxip 6573067
`CStringOwner` → `CStringDropChecker`
qryxip 5e361ba
Update crates/voicevox_core_c_api/src/lib.rs
Hiroshiba f148446
`CStringDropChecker`のdocを更新
qryxip File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
use std::{ | ||
collections::BTreeSet, | ||
ffi::{c_char, CStr, CString}, | ||
sync::Mutex, | ||
}; | ||
|
||
/// `*mut c_char`を`CString`としてdropしていいかチェックする。 | ||
qryxip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// | ||
/// `Mutex`による内部可変性を持ち、すべての操作は共有参照から行うことができる。 | ||
/// | ||
/// # Motivation | ||
/// | ||
/// `CString`は`Box<impl Sized>`と同様Cの世界でもポインタ一つで実体を表すことができるため、こちら側 | ||
/// で管理すべきものは本来無い。しかしながら本クレートが提供するAPIには「解放不要」な文字列を返すも | ||
/// のが含まれている。ユーザーが誤ってそのような文字列を解放するのは未定義動作 (undefined behavior) | ||
/// であるため、綺麗にSEGVするとも限らない。`once_cell::sync::Lazy`由来の文字列の場合、最悪解放が成 | ||
/// 功してしまう。 | ||
/// | ||
/// この構造体はCの世界から帰ってきた`*mut c_char`を`CString`としてdropする際、それが本当にこちら側 | ||
/// が送り出した`CString`かどうかをチェックする。 | ||
/// | ||
/// Cの世界に`CString`を送り出す前に`whitelist`を通し、戻って来た`*mut c_char`を`CString`にしてdrop | ||
/// する前に`check`に通す。 | ||
Comment on lines
+22
to
+23
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. 将来なにか関数増えたときにcheck通すの忘れそうですね。。 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. ですね... ベタ書きしたものをCの世界に向けて発射できる以上、どうしようもない気がします。 |
||
pub(crate) static C_STRING_DROP_CHECKER: CStringDropChecker = CStringDropChecker::new(); | ||
|
||
pub(crate) struct CStringDropChecker(Mutex<Inner>); | ||
|
||
struct Inner { | ||
owned_str_addrs: BTreeSet<usize>, | ||
static_str_addrs: BTreeSet<usize>, | ||
} | ||
|
||
impl CStringDropChecker { | ||
const fn new() -> Self { | ||
Self(Mutex::new(Inner { | ||
owned_str_addrs: BTreeSet::new(), | ||
static_str_addrs: BTreeSet::new(), | ||
})) | ||
} | ||
|
||
/// `CString`をホワイトリストに追加する。 | ||
/// | ||
/// Cの世界に`CString`を送り出す前にこの関数を挟む。 | ||
pub(crate) fn whitelist(&self, s: CString) -> CString { | ||
let Inner { | ||
owned_str_addrs, .. | ||
} = &mut *self.0.lock().unwrap(); | ||
|
||
let duplicated = !owned_str_addrs.insert(s.as_ptr() as usize); | ||
assert!(!duplicated, "duplicated"); | ||
s | ||
} | ||
|
||
/// `&'static CStr`をブラックリストに追加する。 | ||
/// | ||
/// Cの世界に`Lazy`由来の`&'static CStr`を送り出す前にこの関数を挟む。 | ||
/// | ||
/// ホワイトリストとブラックリストは重複しないと考えてよく、ブラックリストはエラーメセージの制御 | ||
/// のためのみに使われる。 | ||
pub(crate) fn blacklist(&self, s: &'static CStr) -> &'static CStr { | ||
let Inner { | ||
static_str_addrs, .. | ||
} = &mut *self.0.lock().unwrap(); | ||
|
||
static_str_addrs.insert(s.as_ptr() as usize); | ||
s | ||
} | ||
|
||
/// `*mut c_char`が`whitelist`を通ったものかどうかチェックする。 | ||
/// | ||
/// # Panics | ||
/// | ||
/// `ptr`が`Self::whitelist`を経由したものではないならパニックする。 | ||
pub(crate) fn check(&self, ptr: *mut c_char) -> *mut c_char { | ||
let Inner { | ||
owned_str_addrs, | ||
static_str_addrs, | ||
.. | ||
} = &mut *self.0.lock().unwrap(); | ||
|
||
let addr = ptr as usize; | ||
if !owned_str_addrs.remove(&addr) { | ||
if static_str_addrs.contains(&addr) { | ||
panic!( | ||
"解放しようとしたポインタはvoicevox_core管理下のものですが、\ | ||
voicevox_coreがアンロードされるまで永続する文字列に対するものです。\ | ||
解放することはできません", | ||
) | ||
} | ||
panic!( | ||
"解放しようとしたポインタはvoicevox_coreの管理下にありません。\ | ||
誤ったポインタであるか、二重解放になっていることが考えられます", | ||
); | ||
} | ||
ptr | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use std::ffi::{c_char, CStr}; | ||
|
||
use super::CStringDropChecker; | ||
|
||
#[test] | ||
#[should_panic( | ||
expected = "解放しようとしたポインタはvoicevox_coreの管理下にありません。誤ったポインタであるか、二重解放になっていることが考えられます" | ||
)] | ||
fn it_denies_unknown_char_ptr() { | ||
let checker = CStringDropChecker::new(); | ||
let s = CStr::from_bytes_with_nul(b"\0").unwrap().to_owned(); | ||
checker.check(s.into_raw()); | ||
} | ||
|
||
#[test] | ||
#[should_panic( | ||
expected = "解放しようとしたポインタはvoicevox_core管理下のものですが、voicevox_coreがアンロードされるまで永続する文字列に対するものです。解放することはできません" | ||
)] | ||
fn it_denies_known_static_char_ptr() { | ||
let checker = CStringDropChecker::new(); | ||
checker.blacklist(STATIC); | ||
checker.check(STATIC.as_ptr() as *mut c_char); | ||
|
||
static STATIC: &CStr = unsafe { CStr::from_bytes_with_nul_unchecked(b"\0") }; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
あくまでチェックということで、
MaybeUninit
への書き込みとCString::{from,into}_raw
をここから外に出しました。メソッド名および概念は"whitelist"(動詞兼名詞)と"blacklist"(動詞兼名詞)としてみました。