Segfault while reading .editorconfig generated by JetBrains IDE about editorconfig-core-c

[greut]

It looks like the packager did enable extra hardening options, I'd take this issue to https://bugs.opensuse.org

[ffes]

My initial guess was that the quite long lines 31, 135, 153 and 171 were the problem. They all are over 2200 chars long. But apparently they were parsed just fine.

The ij_php_array_initializer_new_line_after_left_brace at line 303 mentioned in the trace is 50 chars long, but before that keys longer do exist, for instance ij_coffeescript_method_parameters_new_line_after_left_paren at line 231 is 59 chars long.

Currently in the specs is a hard limit of 50 for keys and 255 for values. There has been a discussion in editorconfig/editorconfig#429 to increase the fixed lengths in the specs, but that hasn't yet let to any merged PR at this moment.
editorconfig/specification#21
editorconfig/editorconfig-core-test#41

[greut]

A easy way to reproduce it.

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index d030664..05970b7 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -85,6 +85,9 @@ if(MSVC)
     add_definitions("-J")
 else()
     add_definitions("-funsigned-char")
+    add_definitions("-D_FORTIFY_SOURCE=1")
+    add_definitions("-O2")
+    add_definitions("-Wall")
 endif()
 
 add_subdirectory(lib)

[Vogtinator]

While #74 avoids the overflow and resulting crash, it means that any of the affected keys are silently ignored or overwrite the values of other keys, which is a misbehaviour. While this only affects files which technically violate the specification, there's no way to tell those apart from valid ones. Please reconsider whether this issue is really fixed.

[xuhdev]

@Vogtinator This is intended to be truncated. See the specification:

The maximum length of a pair key is 50 characters and the maximum length of a pair value is 255 characters. Any key or value beyond these limits shall be ignored.

[ffes]

According to the specs it is fixed. See my comment above for the related issue and PRs about fixing the issue in the specs.

Once the specs are updated the cores should be updated accordingly.

[Vogtinator]

According to the specs it is fixed. See my comment above for the related issue and PRs about fixing the issue in the specs.

No, it's clearly violating the spec. The spec says:

Any key or value beyond these limits shall be ignored.

Truncation is the opposite of ignoring. In this case it assigns the values into a different key.

[xuhdev]

Sorry for the confusion. I think you are right. The test is consistent with your interpretation of the specification (see the relevant lines in limits.in: the test case is testing whether the core library is properly ignoring the 51-char long key).

The C core test is passing this test, which means that it has already been acting properly. The resolving PR merely fixed the overflow issue (the insufficiently allocated array size).

[Vogtinator]

I ran editorconfig through a debugger and can confirm that it's actually ignoring the keys properly indeed.
The reason for my confusion there was that the check for the length happens in ini_parse_file, so before array_editorconfig_name_value_add is actually called. I thought that was not the case, because otherwise the overflow couldn't happen, but there's simply a off-by-one error between the check in ini_parse_file and array_editorconfig_name_value_add which resulted in writing the null-terminator one past the array.

Sorry for the noise and thanks for the quick fix!