Skip to content

Fix Emitte.emitHeader() and add testcase for fragile file names#399

Merged
regisd merged 13 commits intojflex-de:masterfrom
regisd:bad-filename
Oct 7, 2018
Merged

Fix Emitte.emitHeader() and add testcase for fragile file names#399
regisd merged 13 commits intojflex-de:masterfrom
regisd:bad-filename

Conversation

@regisd
Copy link
Member

@regisd regisd commented Sep 30, 2018

  • I realised with commit 97a2b1a that I incorrectly removed a slashify method.
    The initial case to protect against was on Windows for a file called src\main\resources\u0022.txt (or similar).
    Then the generated code comment would be
     // Generated with JFlex
     // source: src\main\resources\u000A.flex
     public class Foo {
    
    which is strictly equivalent to
    // Generated with JFlex
    // source: src\main\resources
    .flex
    public class Foo {
    
    which doesn't compile.
  • The initial fix was to replace all \ by /. Hence both Windows and Unix would produce:
     // source: src/main/resources/u000A.flex
    
    • But that's also incorrect: files in Unix can perfectly have \ in their name – even though that's a bit asking for trouble.
    • So I think we should also escape the \
  • In order to avoid this regression in the future, I also added a test case with a file named src/test/cases/filename/filename\u000AFILE_NAMES_MUST_BE_ESCAPED\u000A.flex that causes (because \u000A is \n):
    [ERROR] .../jflex/testsuite/testcases/src/test/cases/filename/DangerousFileName.java:3: error: class, interface, or enum expected
    [ERROR] // source: src/test/cases/filename/filename\u000AFILE_NAMES_MUST_BE_ESCAPED\u000A.flex
    [ERROR]                                                  ^
    [ERROR] 1 error
    
  • And also, improve the jflex-testsuite-maven-plugin quite a bit:
    • make the execJavac() fail if the input file is missing, to mimic the real behaviour of javac
    • N.B. @lsf37 Several tests didn't compile the generated lexer until now, because toCompile = className + ".java" but the generated lexer doesn't have the name of the test (for instance, it has Yylex.java). This is fixed with execJavac() failing and tests listing their generated files. See commit fb9eb77
    • Use exceptions to propagate the error in the JFlexPTestMojo, instead of a "Test failed!" message.
    • Replace javac-extra-files by javac-files (with the corresponding semantic change) otherwise there is no way to remove the <testname>.java from the input files (even when the flex spec gives a different name)

Nothing is taken after description ?
There is no way to remove the default test.java file otherwise.
(btw, compiling a file which doesn't exist... passes in the jflex-maven-plugin, but not on a real javac)
@regisd regisd added this to the 1.7.1 milestone Sep 30, 2018
@regisd regisd added the bug Not working as intended label Sep 30, 2018
@regisd regisd self-assigned this Sep 30, 2018
… wrong file.

By default, the test tries to compile <Testname>.java, but
- either the lexer is name Yylex.java
- or the camel case rule is incorrect, e.g. No-unused.java
- or the lexer has a completely different name defined by `%xlass` in the flex
@regisd regisd requested a review from lsf37 September 30, 2018 20:42
@regisd regisd changed the title Add testcase for dangerous file names Fix Emitte.emitHeader() and add testcase for fragile file names Sep 30, 2018
Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@regisd regisd merged commit b4ecdc5 into jflex-de:master Oct 7, 2018
@regisd regisd deleted the bad-filename branch October 7, 2018 09:24
@lsf37 lsf37 modified the milestones: 1.7.1, 1.8.0 Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants