Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check string comparison size #1573

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Conversation

mmisono
Copy link
Collaborator

@mmisono mmisono commented Oct 24, 2020

In the debug build, the following assertion failure occurs:

% sudo ./src/bpftrace -e 'BEGIN /comm == "aaaaaaaaaaaaaaaaaaaaaaaaa"/ {}'
bpftrace: /home/ubuntu/work/bpftrace/bpftrace/src/ast/irbuilderbpf.cpp:683: llvm::Value *bpftrace::ast::IRBuilderBPF::CreateStrncmp(llvm::Value *, llvm::Value *, bpftrace::AddrSpace, std::string, uint64_t, const bpftrace::location &, bool): Assertion `valp->getElementType()->isArrayTy() && valp->getElementType()->getArrayNumElements() >= n && valp->getElementType()->getArrayElementType() == getInt8Ty()' failed.

The problem part is https://github.com/iovisor/bpftrace/blob/a03b5c4bbb12a25345a2fdec78374963ff936131/src/ast/irbuilderbpf.cpp#L682
and the reason is that the the size of "comm" is 16, but the size of
string literal is bigger than that.

To fix the problem, check the string size in the semantic analyzer and
error if the size of string literal exeeds the that of the other. Now
the above exmaple becomes

% sudo ./src/bpftrace -e 'BEGIN /comm == "aaaaaaaaaaaaaaaaaaaaaaaaa"/ {}'
stdin:1:7-43: ERROR: The right string literal has the longer length of the size of the left (16 < 25)
BEGIN /comm == "aaaaaaaaaaaaaaaaaaaaaaaaa"/ {}
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Checklist
  • Language changes are updated in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

I wonder if this could cause a regression. I don't think so, but...

src/ast/semantic_analyser.cpp Outdated Show resolved Hide resolved
src/ast/semantic_analyser.cpp Outdated Show resolved Hide resolved
In debug build, the following failure occur:

```
% sudo ./src/bpftrace -e 'BEGIN /comm == "aaaaaaaaaaaaaaaaaaaaaaaaa"/ {}'
bpftrace: /home/ubuntu/work/bpftrace/bpftrace/src/ast/irbuilderbpf.cpp:683: llvm::Value *bpftrace::ast::IRBuilderBPF::CreateStrncmp(llvm::Value *, llvm::Value *, bpftrace::AddrSpace, std::string, uint64_t, const bpftrace::location &, bool): Assertion `valp->getElementType()->isArrayTy() && valp->getElementType()->getArrayNumElements() >= n && valp->getElementType()->getArrayElementType() == getInt8Ty()' failed.
```

The problem part is https:
//github.com/iovisor/bpftrace/blob/a03b5c4bbb12a25345a2fdec78374963ff936131/src/ast/irbuilderbpf.cpp#L682
and the reason is that the the size of "comm" is 16, but the size of
string literal is bigger than that.

To fix the problem, check the string size in the semantic analyzer and
warn if the size of string literal exeeds the that of the other. Now
the above exmaple becomes

```
% sudo ./src/bpftrace -e 'BEGIN /comm == "aaaaaaaaaaaaaaaaaaaaaaaaa"/ {}'  (git)-[fix_string_comparison]
stdin:1:7-43: WARNING: The literal is longer than the variable string (size=16), condition will always be false
BEGIN /comm == "aaaaaaaaaaaaaaaaaaaaaaaaa"/ {}
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
@mmisono
Copy link
Collaborator Author

mmisono commented Oct 27, 2020

Updated:

  • Changed the semantic analyzer so that it emits a warning instead of making an error. Also, make the check simple.
  • In the CreateStrcmp, skip comparison if the condition is always false. Also, fix assertion so that it does not assert that case.

Copy link
Contributor

@fbs fbs 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.

Having a warning test might be useful tho, just to catch this case in a test somewhere.

@mmisono
Copy link
Collaborator Author

mmisono commented Oct 27, 2020

Added test for string comparisons

@fbs fbs merged commit 4a9fac7 into bpftrace:master Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants