add: C APIの`#[repr(Rust)]`なものへのアクセスをすべて安全にする #849

qryxip · 2024-10-05T19:15:11Z

内容

C APIにおいて、Rust APIのオブジェクトそのものの代わりにトークンのような1-bit長の構造体ユーザーに渡すようにすることで、次のことを実現する。

"delete"時に対象オブジェクトに対するアクセスがあった場合、アクセスが終わるまで待つ
次のユーザー操作に対するセーフティネットを張り、パニックするようにする
1. "delete"後に他の通常のメソッド関数の利用を試みる
2. "delete"後に"delete"を試みる
3. そもそもオブジェクトとして変なダングリングポインタが渡される

ドキュメントとしてのsafety requirementsもあわせて緩和する。

このPRは #836 の解決 ではなく、ドキュメントにも手を加えていない。というのもVoicevoxVoiceModelFileには次のゲッターメソッドがあり、これらをカバーするのは現状のAPIの形だと不可能だからである。

~~voicevox_voice_model_file_id~~
~~voicevox_voice_model_file_get_metas_json~~

その他

C APIにおいて次の状況に対してセーフティネットを張り、パニックするようにする。 1. オブジェクトが他スレッドでアクセスされている最中に"delete"を試みる 2. "delete"後に他の通常のメソッド関数の利用を試みる 3. "delete"後に"delete"を試みるこのPRは VOICEVOX#836 の解決**ではなく**、ドキュメントにも手を加えていない。というのも`VoicevoxVoiceModelFile`には次のゲッターメソッドがあり、これらをカバーするのは現状のAPIの形だと不可能だからである。 * `voicevox_voice_model_file_id` * `voicevox_voice_model_file_get_metas_json`

qryxip · 2024-10-05T19:55:05Z

元々やりたかったこととしては #832 みたいな設計をC APIでもやることでした。しかしそのためにはVoicevoxVoiceModelFileに生えている二つのゲッターメソッドが邪魔であることが今回わかりました。(このPRはこのPRでマージするとして、その後)どうにかするための対応としては次のように考えています。

二つのゲッターメソッドをゲッターではなくする

voicevox_voice_model_file_idはuint8_t (*output)[16]のような引数に吐き出すようにし、voicevox_voice_model_file_get_metas_jsonの方はcreate_metas_jsonに改名してvoicevox_json_freeが必要なようにする。
単なるゲッターではなくbracket patternにする

関数オブジェクトをどう表現するかがすごい面倒そう…
char*自体の代わりにVoicevoxStringRefみたいなオブジェクトを返すようにし、VoicevoxStringRefは"delete"を必要とする
```
VoicevoxStringRef *metas = voicevox_voice_model_file_get_metas_json(model);

char *metas_cstr = voicevox_string_ref_get_data(metas);
do_something(metas_cstr);

// `char*`を使い終えたことをVOICEVOX CORE C APIに伝える
voicevox_string_ref_delete(metas);
```
bracket patternほどではないけど使うのが面倒そう… あとVoicevoxStringRefもまた同じように管理するの？という気もしてくる。
[追記] voicevox_voice_model_file_deleteと〃_closeの二種類を作る

Closableや__exit__のようなクローズ処理に対応する"close"と、本当のデストラクトに対応する"delete"の二種類を作る。"close"の方は他言語バインディング作成専用という感じで。
諦める

このPRで対応を打ち切り、「Synthesizer::load_voice_model中にVoiceModelをデストラクトしたらどうなるのか」の問題は各バインディング製作者に任せる。

個人的には1.か4.なのですが、VoicevoxCoreSharpを作っている @yamachu さんにも意見を伺いたいなーと。

https://discord.com/channels/879570910208733277/893889888208977960/1292402538745954346

qryxip · 2024-10-06T08:54:47Z

crates/voicevox_core_c_api/src/c_impls.rs

+/// プロセスの終わりまでデストラクトされない、ユーザーにオブジェクトとして貸し出す`u32`相当の値。
+///
+/// インスタンスは次のような形。
+///
+/// ```
+/// #[derive(Clone, Copy, Debug, From, Into)]
+/// #[repr(Rust)]
+/// pub struct VoicevoxSynthesizer {
+///     id: u32,
+/// }
+/// ```
+///
+/// `Body`そのものではなくこのトレイトのインスタンスをユーザーに渡すようにし、次のユーザー操作に対するセーフティネットを実現する。
+///
+/// 1. オブジェクトが他スレッドでアクセスされている最中に"delete"を試みる
+/// 2. "delete"後に他の通常のメソッド関数の利用を試みる
+/// 3. "delete"後に"delete"を試みる
+///
+/// ただし次のゲッター関数に関しては機能しない。
+///
+/// - `voicevox_voice_model_file_id`
+/// - `voicevox_voice_model_file_get_metas_json`


doc commentだけ書いてみました。
Discordの会話: https://discord.com/channels/879570910208733277/893889888208977960/1292402538745954346

yamachu · 2024-10-06T14:06:36Z

今回の変更ですが、言語バインディングライブラリを作る者として扱いやすいC APIになるなと感じました、ありがとうございます。

#849 (comment)
こちらの VoicevoxVoiceModelFile の扱いですが、私も 1 or 4 、良さそう具合で言えば 1 > 4 かなと考えています。
正直な所、自分の不勉強で申し訳ないのですが 2 がどんな感じになるのかがイメージできてないからという理由もあります。
3 は扱いが面倒だなという一言に尽きるかな…と…

1 は他の API との一貫性が取れているように感じるので、良いのかなとは思いました。

Hiroshiba · 2024-10-06T14:50:51Z

自分もちゃんとわかってない気もしますが、１が良さそうに思いました！
っていうかまあ、これでstring周りの処理が全部共通になるって感じですかね？
確か単純にポイントが得られるパターンと、deleteが必要なパターンがあった記憶。

`VoicevoxVoiceModelFile`の次のゲッター関数を、ゲッター関数ではなくする。 - `voicevox_voice_model_file_id` `uint8_t (*output_voice_model_id)[16]`に吐き出すようにする。 - `voicevox_voice_model_file_get_metas_json` `voicevox_voice_model_file_create_metas_json`に改名。ねらいとしては VOICEVOX#832 のような設計をC APIでも行うことで、サードパーティのバインディング製作者の労力を減らすため。上記のゲッター関数がゲッターではなくなれば、オブジェクトへのアクセスを完全に管理できる。 VOICEVOX#849 (comment)

`VoicevoxVoiceModelFile`の次のゲッター関数を、ゲッター関数ではなくする。 - `voicevox_voice_model_file_id` `uint8_t (*output_voice_model_id)[16]`に吐き出すようにする。 - `voicevox_voice_model_file_get_metas_json` `voicevox_voice_model_file_create_metas_json`に改名。ねらいとしては #832 のような設計をC APIでも行うことで、サードパーティのバインディング製作者の労力を減らすため。上記のゲッター関数がゲッターではなくなれば、オブジェクトへのアクセスを完全に管理できる。 #849 (comment)

qryxip · 2024-10-06T18:16:35Z

464d6f7 (#849): CApiObject::BodyをRustApiObjectに改名しました。

#850 をマージできたので、#836 をやらないとですね…

qryxip · 2024-10-07T18:20:08Z

3e6ddf1 (#849): CApiObjectに"ID"を持たせなくても、ポインタのアドレスで代用できることに気付いたのでそうしてしまいました。永続するboxcar::Vec<impl CApiObject>によるメモリリークが一要素につき4-byteから1-byteになります。
ad753de (#849): "delete"時にオブジェクトへの他のアクセスを待つようにしました。

あとはドキュメントを更新すれば #836 をresolveできるはず。

qryxip · 2024-10-07T20:48:32Z

61cc6df (#849) このPRのdraftを外せるよう、ドキュメントを更新。
e6abd77 (#849): この仕組みならそもそもユーザーから渡されるVoicevoxSynthesizer*とかの有効性を信用しなくてもよいのでは？と思い、API全般においてVoicevoxSynthesizerのポインタはポインタのままで扱うように。これによりユーザーに要求するsafety requirementsが緩和された。

Hiroshiba

一応確認なのですが、レビュー始めちゃっても大丈夫でしょうか 👀

qryxip · 2024-10-08T15:11:10Z

大丈夫です！

crates/voicevox_core_c_api/include/voicevox_core.h

Hiroshiba

LGTM！！！

1点だけ、ドキュメントに関して細かいコメントをしただけになってしまいました！

クラッシュという表現もいい感じな気がしました。もしかしたら最近のプログラマーには通じないのかもだけど。
異常終了が丸いかも。でもまあ何でもいい気がしました！！

VOICEVOX#849 (comment)

0f-0b · 2024-10-24T04:19:47Z

crates/voicevox_core_c_api/src/object.rs

+
+        let this = {
+            let mut heads = Self::lock_heads();
+            heads.push(Default::default());


I noticed that the validate function below checks if the pointer points inside heads, but this line can cause heads to reallocate, which means that creating a new object might invalidate existing pointers.

Oh no thank you! You are obviously right.

I'll use boxcar.

(probably) fixed in #862.

Fix up #849. The main purpose is fixing the following bug using `boxcar`. #849 (comment) Refs: #849

qryxip requested a review from Hiroshiba October 5, 2024 19:15

qryxip force-pushed the add-safety-net-c-api-delete-fuctions branch from 5a56580 to 223a77b Compare October 5, 2024 19:25

CApiObjectにdocを書く

0b38629

https://discord.com/channels/879570910208733277/893889888208977960/1292402538745954346

qryxip commented Oct 6, 2024

View reviewed changes

qryxip mentioned this pull request Oct 6, 2024

change: C APIからゲッター関数を無くす #850

Merged

qryxip added 2 commits October 7, 2024 03:13

Merge branch 'main' into add-safety-net-c-api-delete-fuctions

9cea8fa

CApiObject::{Body → RustApiObject}

464d6f7

qryxip marked this pull request as draft October 6, 2024 18:38

qryxip added 2 commits October 8, 2024 02:33

CApiObjectを1-bit長に

3e6ddf1

drop_body時にbodyの終了を待つようにする

ad753de

qryxip changed the title ~~add: C APIの"delete"にセーフティネットを張る~~ add: C APIの"delete"を安全なAPIにする Oct 7, 2024

ドキュメントとコメントを更新

61cc6df

qryxip force-pushed the add-safety-net-c-api-delete-fuctions branch from 09d4990 to 274c2ea Compare October 7, 2024 20:45

qryxip marked this pull request as ready for review October 7, 2024 20:45

qryxip changed the title ~~add: C APIの"delete"を安全なAPIにする~~ add: C APIの#[repr(Rust)]なものへのアクセスをすべて安全にする Oct 7, 2024

qryxip force-pushed the add-safety-net-c-api-delete-fuctions branch 2 times, most recently from 54d967f to 242e8ed Compare October 8, 2024 02:16

オブジェクトとして渡されるポインタを信頼しない

e6abd77

qryxip force-pushed the add-safety-net-c-api-delete-fuctions branch from 242e8ed to e6abd77 Compare October 8, 2024 02:23

Hiroshiba reviewed Oct 8, 2024

View reviewed changes

crates/voicevox_core_c_api/include/voicevox_core.h Outdated Show resolved Hide resolved

Hiroshiba approved these changes Oct 8, 2024

View reviewed changes

sed -i s/アボート/異常終了/ ./crates/voicevox_core_c_api/src/lib.rs

f69e793

VOICEVOX#849 (comment)

qryxip merged commit f2e6b60 into VOICEVOX:main Oct 8, 2024
35 checks passed

qryxip deleted the add-safety-net-c-api-delete-fuctions branch October 8, 2024 16:38

0f-0b reviewed Oct 24, 2024

View reviewed changes

qryxip added a commit to qryxip/voicevox_core that referenced this pull request Oct 24, 2024

fix: fix up VOICEVOX#849

d45a402

qryxip mentioned this pull request Oct 24, 2024

fix: fix up #849 #862

Merged

qryxip added a commit that referenced this pull request Oct 25, 2024

fix: fix up #849 (#862)

c156ca4

Fix up #849. The main purpose is fixing the following bug using `boxcar`. #849 (comment) Refs: #849

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: C APIの`#[repr(Rust)]`なものへのアクセスをすべて安全にする #849

add: C APIの`#[repr(Rust)]`なものへのアクセスをすべて安全にする #849

qryxip commented Oct 5, 2024 •

edited

Loading

qryxip commented Oct 5, 2024 •

edited

Loading

qryxip Oct 6, 2024

yamachu commented Oct 6, 2024

Hiroshiba commented Oct 6, 2024

qryxip commented Oct 6, 2024 •

edited

Loading

qryxip commented Oct 7, 2024

qryxip commented Oct 7, 2024 •

edited

Loading

Hiroshiba left a comment

qryxip commented Oct 8, 2024

Hiroshiba left a comment

0f-0b Oct 24, 2024

qryxip Oct 24, 2024

qryxip Oct 25, 2024

add: C APIの#[repr(Rust)]なものへのアクセスをすべて安全にする #849

add: C APIの#[repr(Rust)]なものへのアクセスをすべて安全にする #849

Conversation

qryxip commented Oct 5, 2024 • edited Loading

内容

関連 Issue

その他

qryxip commented Oct 5, 2024 • edited Loading

qryxip Oct 6, 2024

Choose a reason for hiding this comment

yamachu commented Oct 6, 2024

Hiroshiba commented Oct 6, 2024

qryxip commented Oct 6, 2024 • edited Loading

qryxip commented Oct 7, 2024

qryxip commented Oct 7, 2024 • edited Loading

Hiroshiba left a comment

Choose a reason for hiding this comment

qryxip commented Oct 8, 2024

Hiroshiba left a comment

Choose a reason for hiding this comment

0f-0b Oct 24, 2024

Choose a reason for hiding this comment

qryxip Oct 24, 2024

Choose a reason for hiding this comment

qryxip Oct 25, 2024

Choose a reason for hiding this comment

add: C APIの`#[repr(Rust)]`なものへのアクセスをすべて安全にする #849

add: C APIの`#[repr(Rust)]`なものへのアクセスをすべて安全にする #849

qryxip commented Oct 5, 2024 •

edited

Loading

qryxip commented Oct 5, 2024 •

edited

Loading

qryxip commented Oct 6, 2024 •

edited

Loading

qryxip commented Oct 7, 2024 •

edited

Loading