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.

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

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Sep 29, 2023

内容

関連 Issue

Resolves #544.

ref #545

その他

  1. 軽い単体テストを書く

と書いていたのですが、意図的に重複を引き当てるケースを作ることが難しいことに気付きました。そのためメッセージの変更のみです。

@qryxip
Copy link
Member Author

qryxip commented Sep 29, 2023

あ、初心者歓迎タスクを自分で付けてたのを忘れてました... まあ他にもまだあるのでよし?

@qryxip
Copy link
Member Author

qryxip commented Sep 29, 2023

なんだこれ... (手元のSphinx v5.3.0はちゃんと動いている)

https://github.com/VOICEVOX/voicevox_core/actions/runs/6355234113/job/17263086343?pr=625

追記: #626 で解決

Copy link
Member

@sevenc-nanashi sevenc-nanashi left a comment

Choose a reason for hiding this comment

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

LGTM!

@qryxip
Copy link
Member Author

qryxip commented Oct 1, 2023

  1. 軽い単体テストを書く

と書いていたのですが、意図的に重複を引き当てるケースを作ることが難しいことに気付きました。そのためメッセージの変更のみです。

これについて補足すると、「重複」が起きる時点でUBであり、それはテストされるべきではないからです。
(ユーザーに対して「やめてください」と言ったはずのことが行われ、その結果Rust側で持っているUnsafeCell<Box<[_]>>がダングリングになる)

テスト側から構造体中のフィールドを弄るという手もあるかもしれませんが、何に対するテストかわからなくなるかと思いました。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

めちゃくちゃ素人なコメントを1つしてます。それが問題なければmergeしちゃって頂けると!!

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

Choose a reason for hiding this comment

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

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

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

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

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自体とかも死ぬ可能性すらある)

@Hiroshiba Hiroshiba merged commit 8209633 into VOICEVOX:main Oct 8, 2023
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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