Debugging symbols please?

The attached backtrace already contains kdevelop's debugging symbols

/usr/lib/x86_64-linux-gnu/libkdevcmakecommon.so: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, BuildID[sha1]=b2ecb624afc81a60ca5fa413e6e9b62c47c9cfec, not stripped
/usr/lib/x86_64-linux-gnu/qt5/plugins/kdevplatform/25/kdevcmakemanager.so: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, BuildID[sha1]=41a85cabcd81f5ae083137f9e3295af248bf3450, not stripped

I can provide backtrace from gdb, but that will tell you the same that crash happened in cmListFileLexer_Scan

you are missing the attachement, I think. I cannot see it at least.

Created attachment 99166 [details]
Backtrace from ASAN

The backtrace (generated by libc) is in the first comment.

Now that I wanted to get the backtrace in gdb, I get crashes in other (unrelated) places.
This is caused by heap corruption, that is detected usually in next free() or realloc() call that can happen in completely different code then the one that caused the memory corruption.

I've recompiled kdevelop with address sanitizer, and now I get reliable bactraces (attached).

Following added assertion in  "cmListFileLexerSetToken" fails:
assert(strlen(text) == length)

thanks, that's much more helpful. Note that libc backtraces don't contain the useful debug information like line number + file. The ASAN build shows everything we need though, ty!

Personally, I think we could resolve this issue in many ways. The best would probably be to do the expensive mime-type check inside the parse job's run method (i.e. in the background thread) to ensure we are not trying to parse binary files. That would guard us against this issue here, and would not lockup the UI thread by doing the mimetype check.

Oh and if it's not clear: the crash is inside generated code (flex? bison? yacc?), so we'd need to fix these tools, or potentially the input file, to prevent that from happening.

Git commit 95497835ee1c721648d34667dd4147d339e8e4c4 by Aleix Pol.
Committed on 25/05/2016 at 23:38.
Pushed by apol into branch '5.0'.

Update the cmake lexer to cmake 3.5.1

Also generate the file with a newer flex version.

M  +1    -1    projectmanagers/cmake/CMakeLists.txt
A  +2761 -0    projectmanagers/cmake/parser/cmListFileLexer.c     [License: GENERATED FILE]  *
D  +0    -2351 projectmanagers/cmake/parser/cmListFileLexer.cpp
M  +33   -24   projectmanagers/cmake/parser/cmListFileLexer.h
M  +224  -27   projectmanagers/cmake/parser/cmListFileLexer.in.l
M  +1    -1    projectmanagers/cmake/parser/cmakelistsparser.cpp

The files marked with a * at the end have a non valid license. Please read: http://techbase.kde.org/Policies/Licensing_Policy and use the headers which are listed at that page.


http://commits.kde.org/kdevelop/95497835ee1c721648d34667dd4147d339e8e4c4

I'm trying to reproduce but I didn't manage. I randomly put a something.txt binary file but I didn't get a crash.

Am I missing something?

(In reply to Aleix Pol from comment #11)
> I'm trying to reproduce but I didn't manage. I randomly put a something.txt
> binary file but I didn't get a crash.
> 
> Am I missing something?

Make sure that KDevelop actually tries to parse the file. You can force it by renaming the filename extension to *.cmake.
If you still won't be able to repro this, I'll try to check what exact byte sequence triggers the crash.

@Maciej: Well, have you been able to reproduce the issue with above patch applied?

Created attachment 99326 [details]
Example crashing file

Attached file causes a crash of kdevelop with ASAN enabled. Since it is only 200 bytes, It most probably won't crash a regular kdevelop build.

The crash still happens with Aleix parser upgrade.

*** Bug 367841 has been marked as a duplicate of this bug. ***

Some comments from the dup:

"""
I found the problem, and it's actually similar, but not the same as bug 363269. The code base being parsed contains a subfolder with example text in a couple of different asian languages, all stored as .txt files in UTF16LE. The cmake parser seems to not just read CMakeLists.txt and *.cmake, but also generally all files with the extension .txt. Apparently the encoding of the example files is handled correctly, but then the wide characters are given to the lexer that expects chars, hence the one byte buffer overflow reported (just my guess, didn't have a look at all the code involved).

If I delete all the UTF16 files, the whole code base is processed successfully.
"""

The crash still happens on todays (09.09.2016) 5.0 branch with UTF-16 txt files. ASAN trace is the same as the one already attached. Without asan it will crash 100% times either during parsing or when saving the cache, so it pretty much blocks any work with such projects.

Strangely enough, I don't see why it's trying to parse random *.txt files (in my case it was 'utf-16.txt' file from reviewboard testdata) - a quick glance at the code says it should only try to parse 'CMakeFiles.txt' or '*.cmake'.

I had some time to kill today, so I took another look at why random .txt files get processed by the background parser. If I understand the code flow correctly, it happens like this:
- on startup the project folder is recursively scanned for all contained files
- every found file is wrapped in a ProjectFileItem (abstractfilemanagerplugin.cpp:234) and added to the projects file set
- the BackgroundParser then creates a ParseJob for every ProjectFileItem (in BackgroundParser::parseDocumentsInternal())
- in BackgroundParser::createParseJob(), the language to use for the current ProjectFileItem is queried by a call to m_languageController->languagesForUrl(qUrl)
- LanguageController::languagesForUrl() in turn checks an internal mimeTypeCache and fileExtensionCache for the language to use for the given file
- in my case, the fileExtensionCache contains an entry that resolves the extension txt to language CMake
- that means in backgroundparser.cpp:357 I get back a CMakeManager, that is then used to create a cmake parse job

Is that expected behaviour or some side effect that has to be fixed? I'm asking this because there are multiple cmake specific spots in the code that explicitly filter for "CMakeLists.txt" and "*.cmake", and that are circumvented by this implicit behaviour.

Thanks for looking. You are indeed right -- .txt files are treated as cmake in some cases. I also poked around a bit, when it does not find a language for a file it looks at the contents, guesses the mime type, and caches that. So when you open CMakeLists.txt, after that all .txt files will be treated as CMake. That is a bug, and needs to be fixed. The relevant code is, I think, in languagecontroller.cpp:310ff.

Git commit 77b83054f943b6c9bce0da178732f7992f7ada3b by Sven Brauch.
Committed on 13/10/2016 at 18:52.
Pushed by brauch into branch '5.0'.

Remove mime type <-> extension cache

The idea that because one file with extension X has mime type A,
determined by its contents, hence other files with extension X will
have the same mime type is just wrong. One common example where this
breaks in a spectacular way is CMakeLists.txt and the .txt extension.

I found the claim that looking into each file will make the application
unresponsive to be unfounded. QMimeType will only read the first 16kB to
guess the mime type, which takes less than a millisecond for each file.
A test project with three hundred 3 MB binary blobs still loads instantly.
If, in comparison, we parse one of the files as CMake erraneously, we
take multiple seconds.
Differential Revision: https://phabricator.kde.org/D3042

M  +1    -35   shell/languagecontroller.cpp

http://commits.kde.org/kdevplatform/77b83054f943b6c9bce0da178732f7992f7ada3b

Note that while above commit should fix the crash in this case, this bug is still open, since it would be back if you would rename the file to "CMakeLists.txt" ... I think.

*** Bug 372575 has been marked as a duplicate of this bug. ***

Created attachment 106865 [details]
Minimal UTF-16 file to trigger behavior

Created attachment 106866 [details]
Proposal for patch

I had another look at the lexer and I think I pinned down the problem with UTF-16 files in cmListFileLexer.c. Scanning the files works fine, but copying the scanned content into the token structure in cmListFileLexerSetToken() is where things go wrong. The code that copies the content uses the functions strcpy() and strdup() that are meant to be used only with zero-terminated character arrays. As we handle two-byte characters, where the most significant byte can be zero (see e.g. letter 'h' with 0x0068), it's possible that the text to be copied to token->text contains zero-bytes mid-string. In that case strdup() doesn't do what it's intended to do. It interprets the buffer as zero-terminated string and only duplicates it up to the first occurence of '\0'. At the same time the original buffer size is stored in token->length. This leads to out-of-bounds memory accesses later on.

I attached a simple UTF-16 file that reliably triggers the problem for me (363269_repro.txt) and a proposal for a fix that replaces the string functions with mallocs/memcpys (363269_proposal.patch). Maybe someone with more experience with the cmake parser/lexer could have a look at it.

Related questions: The patch fixes the crashes/ASAN aborts for me, but is the cmake parser really handling UTF-16 files correctly? Functions like cmListFileLexer_BOM() imply that it can handle all kinds of UTF formats, but at the same time the whole code seems to imply that it only works on zero terminated char arrays. Do we possibly need to update to a newer version of the lexer (which is generated from external sources, right?)?

Does that still happen with KDevelop 5.1+? I thought I fixed it.

Patch still looks sensible to me -- maybe use strncpy instead, and put it on phabricator.kde.org so somebody familiar with the code can have a look?

(In reply to Sven Brauch from comment #27)
> Does that still happen with KDevelop 5.1+? I thought I fixed it.

See comment #22. Random .txt files aren't scanned anymore, but .cmake files can still trigger the faulty behaviour. I conducted my tests with cmListFileLexer.c from master (last updated beginning of the week). But I extracted it into a small reproducer application to ease debugging, so if you fixed it somewhere in the cmake parser outside of cmListFileLexer.c, maybe we don't have to do anything.

(In reply to Sven Brauch from comment #28)
> Patch still looks sensible to me -- maybe use strncpy instead, and put it on
> phabricator.kde.org so somebody familiar with the code can have a look?

strncpy()/strndup() won't work either. Both copy up to n bytes, but also end processing on the occurence of the first '\0' (at least to my understanding).

I'll have a look at phabricator. Never used it before...

Ah yes, if you trick the mimetype detection, you will still crash. You are also right about strncpy.

phabricator should be simple enough to use, just log in (with identity.kde.org), click differential, then click new diff and paste your diff.

(In reply to Sven Brauch from comment #31)
> Ah yes, if you trick the mimetype detection, you will still crash. You are
> also right about strncpy.
> 
> phabricator should be simple enough to use, just log in (with
> identity.kde.org), click differential, then click new diff and paste your
> diff.

OK, I added the diff to phabricator:
https://phabricator.kde.org/D6924

Git commit 3fde9551d5870229d42cc1a61f152fc8d0876b71 by Sven Brauch, on behalf of Axel Kellermann.
Committed on 27/07/2017 at 22:28.
Pushed by brauch into branch '5.1'.

cmListFileLexer: use memcpy instead of strcpy to copy buffer contents to tokens

This fixes a crash when the input is garbage (e.g. binary file which
is not cmake code at all).
Differential Revision: https://phabricator.kde.org/D6924

M  +7    -3    projectmanagers/cmake/parser/cmListFileLexer.c

https://commits.kde.org/kdevelop/3fde9551d5870229d42cc1a61f152fc8d0876b71