require a terminator within maxlen in FIRCLSReadString - #16428
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the 'FIRCLSReadString' function to return false if a null terminator is not found within the specified 'maxlen' range, and adds corresponding unit tests. The review feedback points out a potential crash in the new unit tests where 'strlen' could be called on a NULL pointer if 'FIRCLSReadString' fails, and suggests guarding the assertion to prevent test suite crashes.
| XCTAssertTrue(FIRCLSReadString((vm_address_t)buffer, &string, 32)); | ||
| XCTAssertEqual(string, (char *)buffer); | ||
| XCTAssertEqual(strlen(string), (size_t)31); |
There was a problem hiding this comment.
Since continueAfterFailure is YES by default in XCTestCase, if FIRCLSReadString fails, the test will continue and call strlen(string). Passing NULL to strlen will cause a crash (segmentation fault), which abruptly terminates the test suite run. It is safer to guard the strlen check or assert that string is not NULL before calling strlen.
XCTAssertTrue(FIRCLSReadString((vm_address_t)buffer, &string, 32));
XCTAssertEqual(string, (char *)buffer);
if (string != NULL) {
XCTAssertEqual(strlen(string), (size_t)31);
}2 participants
FIRCLSReadString probes at most maxlen bytes to prove an address points at a readable, terminated string, but when no terminator turns up in that window it still falls through to
*dest = src; return true. It never copies, so the caller gets a pointer it believes is terminated while only maxlen bytes were actually checked. All three call sites in FIRCLSProcess.c then walk it without a bound: the Swift crash_info message (maxlen 256) goes to FIRCLSRedactUUID, which strchrs past the window and writes '*' bytes into it, and then to FIRCLSFileWriteArrayEntryHexEncodedString, which strlens it, so a fatal-error annotation longer than 256 bytes gets neighbouring library memory hex-encoded into the report, or faults inside the handler and costs us the whole report. Moving the success return into the terminator branch keeps the contract the callers already rely on.