[PATCH] D112913: Misleading bidirectional detection

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG35cca45b09b8: Misleading bidirectional detection (authored by serge-sans-paille). Changed prior to commit:

[PATCH] D112913: Misleading bidirectional detection

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:16 +using namespace clang; + +static bool containsMisleadingBidi(StringRef Buffer,

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. In D112913#3233699 , @upsuper wrote: > I'd like to clarify that what I think is correct now is the algorithm to > detect unclosed explicit formatting scopes in a given string. Thanks for confirming. This check only

[PATCH] D112913: Misleading bidirectional detection

upsuper added a comment. I'd like to clarify that what I think is correct now is the algorithm to detect unclosed explicit formatting scopes in a given string. I haven't been following very closely with the whole spoofing issue, so I can't say that there is no other ways to construct a spoof

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. @MaskRay any blocker on that new version now that it recieved a green light from @upsuper? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___ cfe-commits mailing

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 398195. serge-sans-paille added a comment. rebased CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. Note that if there's a consensus on it, I can implement LRM character support. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___ cfe-commits mailing list

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. @aaron.ballman unfortunately I don't know any of those. If I recall correctly we found no software in the RedHat collection actually using those control characters :-/ My understanding is that we are inline with the document you mention, except the fact that

[PATCH] D112913: Misleading bidirectional detection

aaron.ballman added a comment. Has this been tested against any large code bases that use bidirectional characters to see what the false positive rate is? Also, have you seen the latest Unicode guidance on this topic: https://unicode.org/L2/L2022/22007-avoiding-spoof.pdf to make sure we're

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. Thanks @upsuper! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 397885. serge-sans-paille added a comment. rebased on main branch CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112913: Misleading bidirectional detection

upsuper added a comment. I think the core algorithm looks correct now. I'll leave the code review to LLVM reviewers. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___ cfe-commits mailing

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 397813. serge-sans-paille added a comment. Fix some parts of the bidi algorithm, and add extra test cases CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 Files:

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. @upsuper I've not added extra test cases yet, but does it looks it's heading in the right direction? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 397597. serge-sans-paille added a comment. Update bidi algorithm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112913: Misleading bidirectional detection

upsuper added a comment. I'm not familiar with LLVM / Clang codebase. I was asked by @MaskRay to help review the bidi-related part of this patch. Generally I believe the algorithm here is too naive that it produces both false positives and false negatives. I'm not sure how important those edge

[PATCH] D112913: Misleading bidirectional detection

MaskRay added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:49 + if (C == '\n' || C == '\r' || C == '\f' || C == '\v' || + C == 0x85 /*next line*/) +EmbeddingOverride = Isolate = 0; `/*next

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. Gentle ping @MaskRay and/or @rsmith CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 388425. serge-sans-paille added a comment. Nits CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added a comment. Patch rebased on main, all comments addressed. Looks good? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 388424. serge-sans-paille added a comment. Rebase on main branch CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/ https://reviews.llvm.org/D112913 Files: clang-tools-extra/clang-tidy/misc/CMakeLists.txt

[PATCH] D112913: Misleading bidirectional detection

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:105 + + Inspect string literal and comments for unterminated bidirectional Unicode + characters. Nit: Inspects CHANGES SINCE LAST ACTION

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:59 +// If conversion fails, utf-8 is designed so that we can just try next char. +if (Result != llvm::conversionOK) { + ++CurPtr;

[PATCH] D112913: Misleading bidirectional detection

rsmith added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:59 +// If conversion fails, utf-8 is designed so that we can just try next char. +if (Result != llvm::conversionOK) { + ++CurPtr; Is there a

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille updated this revision to Diff 384111. serge-sans-paille added a comment. - recover from failed utf8 decoding - doc and release note updated - clang-formatting - more examples / testing CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112913/new/

[PATCH] D112913: Misleading bidirectional detection

MaskRay added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/misc-misleading-bidirectional.cpp:1 +// RUN: %check_clang_tidy %s misc-misleading-bidirectional %t + The test misses many interesting cases. Repository: rG LLVM Github

[PATCH] D112913: Misleading bidirectional detection

MaskRay requested changes to this revision. MaskRay added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MisleadingBidirectional.cpp:19 + +static bool ContainsMisleadingBidi(StringRef Buffer, bool HonorLineBreaks=true) { + const char* CurPtr = Buffer.begin();

[PATCH] D112913: Misleading bidirectional detection

MaskRay added a comment. > Context not available. If you don't use `arc diff 'HEAD^'` to upload a Diff, please use -U9 https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface Comment at:

[PATCH] D112913: Misleading bidirectional detection

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:61 +CheckFactories.registerCheck( +"misc-misleading-bidirectional"); } Nit: please keep alphabetical order in the list of jobs. Repository:

[PATCH] D112913: Misleading bidirectional detection

carlosgalvezp requested changes to this revision. carlosgalvezp added a comment. This revision now requires changes to proceed. Nice addition! Please add this check to the documentation (list of available checks + individual page with the documentation for this check), plus mention in the

[PATCH] D112913: Misleading bidirectional detection

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Nits and a suggested approach for invalid code sequences that's probably important to handle better. Please fix the clang-tidy findings too. Otherwise, LGTM. Comment at:

[PATCH] D112913: Misleading bidirectional detection

serge-sans-paille created this revision. Herald added subscribers: carlosgalvezp, mgorny. serge-sans-paille requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. This patch implements detection of incomplete bidirectional sequence