fix: harden runtime FST and JNI handling - #377
Conversation
pengzhendong
left a comment
There was a problem hiding this comment.
Thanks for fixing the JNI UTF string leak and checking both FST reads. The build matrix is green, and the new Processor load test passes, but I think two JNI correctness gaps still need to be closed before merge:
normalizeandinverse_normalizedereference the global processor pointers without checking initialization. Calling either method beforeinit, or after the newinitfailure path resets both pointers, can still cause a native null dereference instead of a Java exception.- The processing JNI methods do not catch C++ exceptions.
Tag/Verbalizecan throw (the token parser already has malformed-input exception paths), andCopyJStringcan also throw during allocation. A C++ exception escaping a JNI native method can terminate the process.
Minimum requested changes:
- Guard the corresponding processor at both processing entry points; throw a Java
IllegalStateExceptionand returnnullptrwhen it is unavailable. - Wrap each complete processing body in an exception boundary and translate
std::exception(and preferably unknown exceptions) to a Java exception. - Add coverage for normalize-before-init / after-failed-init and for the verbalizer FST read failure branch.
Strong follow-up: construct TN and ITN processors in local variables and publish them only after both constructors succeed, so a failed re-initialization does not discard or temporarily expose partial state. The existing unsynchronized global state is older debt; I am not treating the full concurrency redesign as a blocker for this PR.
| jstring normalize(JNIEnv* env, jobject, jstring input) { | ||
| std::string input_text = std::string(env->GetStringUTFChars(input, nullptr)); | ||
| std::string input_text; | ||
| if (!CopyJString(env, input, &input_text)) { |
There was a problem hiding this comment.
[P1] Please guard processorTN before the later dereference. init now resets both processors when loading fails, so calling normalize before initialization or after a handled initialization failure still reaches a native null dereference. Throw IllegalStateException and return nullptr instead.
| jstring inverse_normalize(JNIEnv* env, jobject, jstring input) { | ||
| std::string input_text = std::string(env->GetStringUTFChars(input, nullptr)); | ||
| std::string input_text; | ||
| if (!CopyJString(env, input, &input_text)) { |
There was a problem hiding this comment.
[P1] Please put the complete body of both processing JNI methods inside a C++ exception boundary. Tag/Verbalize can throw (including malformed-token errors), and even the string copy may throw during allocation; letting those exceptions escape the JNI frame can terminate the app. Translate them to a Java exception and return nullptr.
| TEST(ProcessorLoadTest, ThrowsWhenFstFilesCannotBeLoaded) { | ||
| EXPECT_THROW( | ||
| wetext::Processor("/tmp/zh_tn_missing_tagger.fst", | ||
| "/tmp/zh_tn_missing_verbalizer.fst"), |
There was a problem hiding this comment.
[P2] This case throws on the missing tagger, so it never exercises the new verbalizer null check. Please add a valid-tagger + missing-verbalizer case. It would also be better to use a portable unique temporary path instead of fixed /tmp names.
Summary
Verification
Notes
The full FST-backed runtime test set was not run locally because this environment has Python 3.14 and no compatible Pynini installation to generate the required model artifacts.