Skip to content
This repository has been archived by the owner on Jul 30, 2020. It is now read-only.

Crash on code using default argument for template template parameter #219

Closed
HighCommander4 opened this issue Jan 2, 2018 · 16 comments
Closed

Comments

@HighCommander4
Copy link
Contributor

When indexing a project containing a single code file containing just the following include:

#include <boost/phoenix/core/argument.hpp>

the indexer crashes with:

libclang: crash detected during indexing TU
2018-01-02 18:08:12.301 (   3.261s) [indexer4        ]             indexer.cc:1890  WARN| Indexing /home/nr/dev/projects/cxx/test/src/test.cpp failed with errno=1

If I remove the include, there is no crash. I'm using Boost 1.63.

@HighCommander4
Copy link
Contributor Author

I am using compile_commands.json, which looks like this:

[
{
  "directory": "/home/nr/dev/projects/cxx/test",
  "command": "clang++ -otest.o -c src/test.cpp -I/home/nr/dev/dist/boost/include -std=c++14 -Wall -O3 -fcolor-diagnostics",
  "file": "/home/nr/dev/projects/cxx/test/src/test.cpp"
}
]

@HighCommander4
Copy link
Contributor Author

Reduced code example that triggers the crash:

template <typename>
struct actor;

template <template <typename> class Actor = actor>
struct terminal;

If the default template argument is removed, the crash goes away.

@HighCommander4 HighCommander4 changed the title Crash when indexing Boost.Phoenix header Crash on code using default argument for template template parameter Jan 2, 2018
@jiegec
Copy link
Contributor

jiegec commented Jan 3, 2018

I can reproduce this:

2018-01-03 08:28:41.790 (   0.092s) [indexer0        ]             indexer.cc:1890  WARN| Indexing /Volumes/Data/small_programs/template_default_argument.cc failed with errno=1

, after which:

2018-01-03 08:30:58.901 ( 137.198s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.
2018-01-03 08:31:00.176 ( 138.473s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.
2018-01-03 08:31:00.178 ( 138.475s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.
2018-01-03 08:31:00.402 ( 138.700s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.
2018-01-03 08:31:00.404 ( 138.701s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.
2018-01-03 08:31:00.405 ( 138.702s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.
2018-01-03 08:31:00.406 ( 138.704s) [querydb         ]     message_handler.cc:46       0| "/Volumes/Data/small_programs/template_default_argument.cc" is being indexed.

Building the file itself just went fine:

template <typename>
struct actor;

template <template <typename> class Actor = actor>
struct terminal;

int main() {
    return 0;
}

@MaskRay
Copy link
Contributor

MaskRay commented Jan 3, 2018

I can reproduce with HighCommander4's example too.

If you uncomment clang_toggleCrashRecovery before clang_indexTranslationUnit in src/indexer.cc

1880
  clang_toggleCrashRecovery(0);  /////// uncomment this statement
  // |index_result| is a CXErrorCode instance.
  int index_result = clang_indexTranslationUnit(

It reveals this is a bug on libclang's side (--bundled-clang=5.0.1), probably similar to #192

Linking cquery with cmake -DCMAKE_BUILD_TYPE={Debug,Release} llvm+clang git HEAD does not have this issue. Probably fixed in upstream.

Before we get a new release on releases.llvm.org, you may consider linking cquery against clang+llvm built from the source:

CXXFLAGS="-g -I$HOME/Dev/llvm/tools/clang/include" ./waf configure --variant=my-clang-release --use-system-clang --llvm-config=$HOME/Dev/llvm/release/bin/llvm-config

@MaskRay
Copy link
Contributor

MaskRay commented Jan 3, 2018

May be a race in libclang.

printf '\x4d' | dd of=build/release/lib/clang+llvm-5.0.1-x86_64-linux-gnu-ubuntu-14.04/lib/libclang.so.5.0 obs=1 seek=$[0x47aece] conv=notrunc seems to suppress this issue

@HighCommander4
Copy link
Contributor Author

I can confirm that the workaround described in the previous comment prevents the crash.

@MaskRay, out of curiosity, how do you come up with a binary patch like this? Do you start with a source change to libclang, and take a diff of the resulting binaries? Or do you somehow debug the binary directly?

@MaskRay
Copy link
Contributor

MaskRay commented Jan 3, 2018

I leave some notes in https://maskray.me/blog/2017-12-25-cquery-updates-and-libclang-one-byte-patch (sorry, in Chinese).

@jiegec
Copy link
Contributor

jiegec commented Jan 3, 2018

@HighCommander4 He... participated in DEFCON CTF Contest... His team won World No.5. Binary manipulation is no less familiar to him than cplusplus.

@HighCommander4
Copy link
Contributor Author

Impressive :) IIUC (I ran the blog post through Google Translate), it starts with a desired source change, but additional cleverness is required to find a way to express the source change as a binary change of only 1 byte.

Anyways, what I really wanted to ask is, do you know the source change in this case? If so, could you share it?

@MaskRay
Copy link
Contributor

MaskRay commented Jan 3, 2018

Just translated most part of that article.... And the patch is sent for review at https://reviews.llvm.org/D41575

@HighCommander4
Copy link
Contributor Author

HighCommander4 commented Jan 3, 2018

Oh, I just noticed it's the same change as in #192 (adding a null check).

@MaskRay
Copy link
Contributor

MaskRay commented Jan 3, 2018

The libclang handleReference null pointer dereference issue is still weird in that it occurs sometimes but not always. I have never seen it in a debug build and the issue also disappears if I attach the process with gdb. I assume it may be caused by some race condition.

@MaskRay
Copy link
Contributor

MaskRay commented Jan 6, 2018

Mark as resolved as the hack is commited #226 in wscript

There is still no response from the clang upstream https://reviews.llvm.org/D41575 . What a high threshold for newcomers. There are definitely duplicated efforts spending on cquery and clangd. I'm still unclear what will happen. #63

@MaskRay MaskRay closed this as completed Jan 6, 2018
@HighCommander4
Copy link
Contributor Author

There is still no response from the clang upstream https://reviews.llvm.org/D41575 .

Perhaps you could submit the patch to their Review Corner?

spurious pushed a commit to spurious/clang-mirror that referenced this issue Jan 8, 2018
Summary:
DC may sometimes be NULL and getContainerInfo(DC, Container) will dereference a null pointer.

Default template arguments (the following example and many test files in https://github.com/nlohmann/json)
may cause null pointer dereference.

```c++
template <typename>
struct actor;

template <template <typename> class Actor = actor>
struct terminal;
```

In tools/libclang/CXIndexDataConsumer.cpp#L203

    handleReference(ND, Loc, Cursor,
                    dyn_cast_or_null<NamedDecl>(ASTNode.Parent),
                    ASTNode.ContainerDC, ASTNode.OrigE, Kind);

`dyn_cast_or_null<NamedDecl>(ASTNode.Parent)` is somehow a null pointer and in tools/libclang/CXIndexDataConsumer.cpp:935

  ContainerInfo Container;
  getContainerInfo(DC, Container);

The null DC is casted `ContInfo.cursor = getCursor(cast<Decl>(DC));` and SIGSEGV.

```

See discussions in jacobdufault/cquery#219 jacobdufault/cquery#192

Reviewers: akyrtzi, sammccall, yvvan

Reviewed By: sammccall

Subscribers: mehdi_amini, cfe-commits

Differential Revision: https://reviews.llvm.org/D41575

git-svn-id: http://llvm.org/svn/llvm-project/cfe/trunk@322017 91177308-0d34-0410-b5e6-96231b3b80d8
jyknight pushed a commit to jyknight/llvm-monorepo that referenced this issue Jan 8, 2018
Summary:
DC may sometimes be NULL and getContainerInfo(DC, Container) will dereference a null pointer.

Default template arguments (the following example and many test files in https://github.com/nlohmann/json)
may cause null pointer dereference.

```c++
template <typename>
struct actor;

template <template <typename> class Actor = actor>
struct terminal;
```

In tools/libclang/CXIndexDataConsumer.cpp#L203

    handleReference(ND, Loc, Cursor,
                    dyn_cast_or_null<NamedDecl>(ASTNode.Parent),
                    ASTNode.ContainerDC, ASTNode.OrigE, Kind);

`dyn_cast_or_null<NamedDecl>(ASTNode.Parent)` is somehow a null pointer and in tools/libclang/CXIndexDataConsumer.cpp:935

  ContainerInfo Container;
  getContainerInfo(DC, Container);

The null DC is casted `ContInfo.cursor = getCursor(cast<Decl>(DC));` and SIGSEGV.

```

See discussions in jacobdufault/cquery#219 jacobdufault/cquery#192

Reviewers: akyrtzi, sammccall, yvvan

Reviewed By: sammccall

Subscribers: mehdi_amini, cfe-commits

Differential Revision: https://reviews.llvm.org/D41575

llvm-svn=322017
@MaskRay
Copy link
Contributor

MaskRay commented Jan 8, 2018

There is still no response from the clang upstream https://reviews.llvm.org/D41575 .

Perhaps you could submit the patch to their Review Corner?

Thanks for the pointer! I'll use that if there is a bug next time :) Commited in https://reviews.llvm.org/rC322017

@Yevgnen
Copy link

Yevgnen commented Jan 19, 2018

@MaskRay Will the binary patch work for libclang.dylib on macOS?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants