Conversation
* Add validation for `wpsReg` command response in `WpsExecutor` to catch initialization failures. * Enhance `pixiewps_wrapper.c` to handle cases where the process is killed by a signal and improve exit status logging. * Update `WpsResult` to recognize `msg=8` (M4 failure) and `msg=10` (M6 failure) strings for more accurate PIN progress tracking. * Add comprehensive unit tests in `WpsResultTest` to verify the mapping of native status outputs to Java result states (success, timeout, locked, etc.).
There was a problem hiding this comment.
Pull request overview
This PR improves robustness and observability of the NDK-based WPS workflows by tightening native/Java result interpretation and adding tests that validate WPS status mapping and PIN progress detection.
Changes:
- Extend
WpsResult.isFirstHalfCorrect()to detect M4/M6 failures viamsg=8/msg=10. - Add
wpsRegresponse validation inWpsExecutorto fail fast on initialization errors. - Improve
pixiewps_wrapper.chandling/logging when pixiewps is terminated by a signal, and add unit tests covering native-status-to-Java mapping.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/src/test/java/sangiorgi/wps/lib/services/WpsResultTest.java |
Adds unit tests that simulate convertNativeResult() outputs and verify WpsResult state mapping. |
lib/src/main/java/sangiorgi/wps/lib/services/WpsResult.java |
Enhances first-half PIN correctness detection by recognizing msg=8/msg=10. |
lib/src/main/java/sangiorgi/wps/lib/services/WpsExecutor.java |
Checks wpsReg response to detect failures earlier in the WPS flow. |
lib/src/main/cpp/pixiewps/pixiewps_wrapper.c |
Improves handling/logging for signal-killed pixiewps processes and clarifies exit status logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (regResponse == null) { | ||
| return createErrorResult(bssid, pin, "WPS_REG command failed (invalid input or shell not running)"); |
There was a problem hiding this comment.
wpsNative.wpsReg() returns a trimmed string and can return an empty string (or a failure token like FAIL) without being null. The current check only handles null, so we may proceed to readWpsResult() even when WPS_REG never actually started (initialization/control socket not ready). Consider treating blank/whitespace responses and known failure responses (e.g., FAIL) as an error and returning a createErrorResult, ideally including the response text for diagnostics.
| if (regResponse == null) { | |
| return createErrorResult(bssid, pin, "WPS_REG command failed (invalid input or shell not running)"); | |
| String trimmedResponse = regResponse != null ? regResponse.trim() : null; | |
| if (trimmedResponse == null | |
| || trimmedResponse.isEmpty() | |
| || "FAIL".equalsIgnoreCase(trimmedResponse)) { | |
| String responseText = String.valueOf(regResponse); | |
| return createErrorResult( | |
| bssid, | |
| pin, | |
| "WPS_REG command failed (response: " + responseText + ")"); |
| int status; | ||
| waitpid(pid, &status, 0); | ||
|
|
||
| if (WIFSIGNALED(status)) { | ||
| LOGE("pixiewps_compute: killed by signal %d, path=%s", WTERMSIG(status), pixiewps_exec_path); | ||
| pin_out[0] = '\0'; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
waitpid(pid, &status, 0) return value isn’t checked. If waitpid fails, status is undefined and the subsequent WIFSIGNALED/WIFEXITED handling and logging can read garbage. Handle waitpid returning -1 (log errno, clear pin_out, close resources) before interpreting status.
* Strengthen response validation in `WpsExecutor` to handle empty or "FAIL" results from `wpsReg`. * Add `waitpid` error checking and logging to `pixiewps_wrapper.c` to better handle process execution failures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public boolean isFirstHalfCorrect() { | ||
| for (CommandResult result : commandResults) { | ||
| String output = result.getOutputAsString().toLowerCase(Locale.ROOT); | ||
| // M6 failure indicates first half was correct | ||
| if (output.contains("m6") || output.contains("wsc_nack after m6")) { | ||
| // M6 failure (msg=10) indicates first half was correct, second half wrong | ||
| if (output.contains("m6") || output.contains("wsc_nack after m6") | ||
| || output.contains("msg=10")) { | ||
| return true; | ||
| } | ||
| // M4 failure indicates first half was wrong | ||
| if (output.contains("m4") || output.contains("wsc_nack after m4")) { | ||
| // M4 failure (msg=8) indicates first half was wrong | ||
| if (output.contains("m4") || output.contains("wsc_nack after m4") | ||
| || output.contains("msg=8")) { | ||
| return false; |
There was a problem hiding this comment.
isFirstHalfCorrect() returns immediately when it finds an M4/msg=8 indicator in an earlier CommandResult, which can incorrectly report false if a later aggregated result contains an M6/msg=10 indicator (first half correct). Since WpsResult can aggregate multiple command outputs, consider scanning all results first (e.g., remember whether any M6/msg=10 was seen, then only fall back to M4/msg=8 if no M6 was found) rather than returning on the first match.
* Split the result scanning into two passes to ensure an M6 failure (indicating the first half is correct) takes precedence over an M4 failure. * This ensures correct evaluation when `WpsResult` aggregates multiple command results, such as in Pixie Dust attacks.
wpsRegcommand response inWpsExecutorto catch initialization failures.pixiewps_wrapper.cto handle cases where the process is killed by a signal and improve exit status logging.WpsResultto recognizemsg=8(M4 failure) andmsg=10(M6 failure) strings for more accurate PIN progress tracking.WpsResultTestto verify the mapping of native status outputs to Java result states (success, timeout, locked, etc.).