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

Fix a possible integer overflow #1580

Merged
merged 1 commit into from
Nov 3, 2020
Merged

Conversation

mmisono
Copy link
Collaborator

@mmisono mmisono commented Oct 28, 2020

When I tried to use UndefinedSanitizer (by
CXXFLAGS="-fsanitize=undefined"), I found the following error.

% sudo ./src/bpftrace -e 'BEGIN{curtask}'
/home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:22: runtime error: shift exponent 64 is too large for 32-bit type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:22 in
/home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:44: runtime error: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:44 in

It warned about integer overflows. This patch fixes this.

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

@@ -222,7 +222,10 @@ static bool getBitfield(CXCursor c, Bitfield &bitfield)
size_t bitfield_offset = clang_Cursor_getOffsetOfField(c) % 8;
size_t bitfield_bitwidth = clang_getFieldDeclBitWidth(c);

bitfield.mask = (1 << bitfield_bitwidth) - 1;
if (bitfield_bitwidth == 64)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be >=32 as the warning is:

shift exponent 64 is too large for 32-bit type 'int'

Copy link
Member

Choose a reason for hiding this comment

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

of maybe

if (width >= sizeof(size_t)) {
  mask = SSIZE_MAX
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shouldn't this be >=32 as the warning is:

Isn't bitfield_bitwidth >= 32 possible? something like struct {long x: 48; long y: 16}

of maybe

if (width >= sizeof(size_t)) {
  mask = SSIZE_MAX
}

ah, this seems better. will fix.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we make bitfield.mask an uint64_t then, to avoid issues on 32bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes sense. I also noticed that both clang and gcc support __int128. So I added the support of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I realized that bpftrace does not support __int128. So, change it to use uint64_t

@mmisono
Copy link
Collaborator Author

mmisono commented Oct 29, 2020

hmm.. it seems the error reason for the embedded build is not related to this.

/home/runner/work/bpftrace/bpftrace/src/attached_probe.cpp: In member function 'void bpftrace::AttachedProbe::attach_uprobe(bool)':
/home/runner/work/bpftrace/bpftrace/src/attached_probe.cpp:706:51: error: too few arguments to function 'int bpf_attach_uprobe(int, bpf_probe_attach_type, const char*, const char*, uint64_t, pid_t, uint32_t)'
                                         probe_.pid);
                                                   ^
[...]
```

When I tried to use UndefinedSanitizer (by
CXXFLAGS="-fsanitize=undefined"), I found the following error.

```
% sudo ./src/bpftrace -e 'BEGIN{curtask}'
/home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:22: runtime error: shift exponent 64 is too large for 32-bit type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:22 in
/home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:44: runtime error: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/ubuntu/work/bpftrace_master/src/clang_parser.cpp:225:44 in
```

It warned about integer overflows. This patch fixes this.
@fbs fbs merged commit 1e39a01 into bpftrace:master Nov 3, 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