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

safer version of text2mecab #4

Merged
merged 5 commits into from
Apr 13, 2022
Merged

Conversation

Yosshi999
Copy link

cf. VOICEVOX/voicevox_engine#384 (comment)

CRT のセキュリティ機能 を参考に、
buffer overflowの危険性があるtext2mecabに対し、バッファーサイズを指定してそれをはみ出した場合エラーが返る text2mecab_s を実装した。
以下のような関数呼び出しをこれに置き換え、よりメモリ安全にする。

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.

VOICEVOXエンジン側で使っているopenjtalkの関数は、確保したメモリサイズ以上の領域に書き込む危険性があるので、その領域に書き込もうとした前にエラーを返すような関数を追加した、という文脈でしょうか。

なるほどです!!
text2mecab_sを追加するのではなく、text2mecabを書き換えてしまうという手もあるかもと感じました。
openjtalkに変更が加わえられたときに追従しやすいこと、たぶん安全じゃない関数の需要はないことが主な理由です。
まあその場合、text2mecabが使われている箇所全部を変更する必要がありますが・・・。

どうでしょうか👀

@Yosshi999
Copy link
Author

あまりopen_jtalkに変更を加えたくなかったのですが、そちらでも全然ありだと思います

@Hiroshiba
Copy link
Member

ありがとうございます!
そちら(上書き)の実装に変更していただけると助かります!

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!!

@Hiroshiba
Copy link
Member

これをENGINEに反映させるにはいろいろ更新が必要かもですね・・・
せっかくなので、もしよかったらお願いしたいです・・・!

@Hiroshiba Hiroshiba merged commit d74d20a into VOICEVOX:1.11 Apr 13, 2022
@Yosshi999
Copy link
Author

このPRの変更点はextract_fullcontextという関数に包まれているので、ENGINE における変更点はpyopenjtalk変更に伴うrequirements.txtの修正のみになりそうです。

@Yosshi999
Copy link
Author

Yosshi999 commented Apr 21, 2022

追従TODO

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.

2 participants