On Tue, 21 Oct 2025 16:24:48 GMT, Erik Joelsson <[email protected]> wrote:
>> Daniel Hu has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - fix incorrect use of inputstream >> - remove extraneous variables/imports > > I took a look at this again and I appreciate the effort you've put in. That > said, the code is now quite convoluted and hard to understand. Would you mind > adding some comments describing the algorithm on a high level and perhaps > what each method is meant to accomplish? > > I would also like to know what verification of detection of failures you have > done. If you deliberately allow absolute paths in the build, will the test > properly detect them? The drawback of a complicated implementation is that it > becomes hard to validate the test itself. I'm almost thinking we need a test > for the test here, that you could invoke manually to make sure it detects > positives. Specifically for corner cases when offending strings cross buffer > boundaries. > > Will the new implementation handle a search pattern longer than the buffer > size? I doubt we would ever need such a long pattern, but if not, perhaps the > buffer size should be chosen dynamically to accommodate? @erikj79 Yes, more than glad to explain! I forgot to leave comments earlier/got stuck with security release work yesterday. Let me know if any of the comments are missing in explanation I definitely agree that more testing could be done. I probably should've figured this out before taking this task, but do you know how to build the jdk deliberately with absolute paths? (or if you have any download links to broken/incorrect jdk builds, that would also work). I did test the string searching algorithm itself, by adding a mundane string pattern already in the build files (e.g. "java") and the test successfully failed/matched existing strings in a working jdk build. Yes, the implementation works for any sized search pattern regardless of buffer size (thanks to the chosen string search algorithm). The separate function for printing output actually already has a dynamic buffer (`ByteArrayOutputStream output`), and fortunately it's not even used when the test passes. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26030#issuecomment-3433577891
