fix(random): random_buffer should return an error code instead of abort
#4621
Conversation
* TODO consider refactoring WASM Random.cpp
There was a problem hiding this comment.
Pull request overview
This pull request refactors the random_buffer API to return error codes instead of aborting on failure, improving error handling across all platform implementations and call sites.
Changes:
- Changed
random_bufferfunction signature to returnint(0 for success, <0 for error) across all platform implementations - Updated all call sites to check return values and handle errors appropriately
- Enhanced JNI implementation with RAII-style resource management and proper exception handling
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rand.h | Added documentation for return value semantics (0 on success, <0 on error) |
| src/rand.cpp | Linux implementation now returns error codes instead of throwing exceptions |
| swift/Sources/SecRandom.m | Swift/macOS implementation returns -1 on failure instead of calling abort() |
| wasm/src/Random.cpp | WASM implementation updated to return 0 on success |
| jni/cpp/Random.cpp | Major refactor with JNIEnvContext class for safe resource management and error handling |
| src/interface/TWPrivateKey.cpp | Returns nullptr on random generation failure instead of calling std::terminate() |
| src/Keystore/ScryptParameters.cpp | Throws exception with error message if salt generation fails |
| src/Keystore/PBKDF2Parameters.cpp | Throws exception with error message if salt generation fails |
| src/Keystore/AESParameters.cpp | Throws exception with error message if IV generation fails |
| src/FIO/Encryption.cpp | Throws exception with error message if IV generation fails |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ssize_t readLen = read(randomData, buf, len); | ||
| close(randomData); | ||
| if (readLen != static_cast<ssize_t>(len)) { |
There was a problem hiding this comment.
The comparison readLen != static_cast<ssize_t>(len) doesn't distinguish between partial reads and actual errors. A negative readLen indicates an error, while a positive value less than len indicates a short read. For /dev/urandom, short reads are possible if interrupted by a signal. Consider checking if (readLen < 0 || readLen != static_cast<ssize_t>(len)) or handling EINTR with a retry loop for robustness.
There was a problem hiding this comment.
TODO!
Binary size comparison➡️ aarch64-apple-ios: 15.25 MB ➡️ aarch64-apple-ios-sim: 15.25 MB ➡️ aarch64-linux-android: 20.10 MB ➡️ armv7-linux-androideabi: 17.31 MB ➡️ wasm32-unknown-emscripten: 14.85 MB |
This pull request refactors the random buffer generation logic across multiple platforms to improve error handling and consistency. The main change is that the
random_bufferfunction now returns an integer status code (0 for success, <0 for error) instead of being void, and all call sites now check for errors and handle them appropriately. Additionally, the JNI implementation is significantly improved for safety and resource management.Random buffer API refactor:
random_bufferfunction signature is changed to return anintstatus code across all platform implementations (jni/cpp/Random.cpp,src/rand.cpp,swift/Sources/SecRandom.m,wasm/src/Random.cpp). Callers now check the return value and handle errors instead of assuming success.Error handling improvements:
random_buffernow check its return value. If random data generation fails, they throw exceptions or return error codes instead of proceeding with invalid data. This affects IV and salt generation in encryption and keystore modules, and private key creation.JNI implementation enhancements:
JNIEnvContextclass for safe environment management, proper reference handling, and exception checking. The implementation now returns error codes on failure rather than throwing or crashing.Interface documentation and consistency:
src/rand.his updated to document the new return value semantics forrandom_buffer, ensuring consistent usage and expectations across the codebase.Platform-specific improvements:
These changes improve the robustness and safety of cryptographic operations by ensuring that random data generation failures are properly detected and handled.