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

Crash when change "Trigger" "Slope" to "up&down" from just being up, if trigger set to left edge of screen #278

Closed
ericfont opened this issue Mar 24, 2022 · 16 comments
Labels
BUG Something isn't working

Comments

@ericfont
Copy link
Contributor

ericfont commented Mar 24, 2022

Describe the bug
Crash when change "Trigger" "Slope" to "up&down" from just being up, when I am viewing a 25% duty-cycle 3.3v square wave in channel 2 with twigger mode "Auto" with trigger source "CH2".

OpenHantek6022 (20220317 - commit 971)
"4.6 (Compatibility Profile) Mesa 21.3.7"
Graphic: 4.6 (Compatibility Profile) Mesa 21.3.7 - GLSL version 1.20
/usr/include/c++/11.2.0/bits/stl_vector.h:1063: std::vector<_Tp, _Alloc>::const_reference std::vector<_Tp, _Alloc>::operator[](std::vector<_Tp, _Alloc>::size_type) const [with _Tp = double; _Alloc = std::allocator<double>; std::vector<_Tp, _Alloc>::const_reference = const double&; std::vector<_Tp, _Alloc>::size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted (core dumped)

To Reproduce

  1. Have voltage display for CH1 & CH2 enabled.
  2. make a 3.3v square wave in CH2.
  3. have trigger level in the middle voltage of the 3.3v square wave @3.613 kHz.
  4. use timebase of 200 us with samplerate 5 MS/s.
  5. set trigger mode to "Auto" with source "CH2" with slope "up" with any filter mode.
  6. Then crash will happen when change trigger "Slope" from "up" to "up&down".

Expected behavior
no crash

Screenshots
before crash:
image

Computer environment (please complete the following information):

  • OpenHantek version: self-compile release commit 971
  • OS: Linux 5.15.30-1-lts
  • Distribution, version: Arch Linux latest rolling version x86-64.
  • Video hardware: Radeon RX 560
  • OpenGL version: 4.6 (Compatibility Profile) Mesa 21.3.7
  • Qt version: Qt 5.15.3
  • C++ compiler version: gcc 11.2.0

Scope device (please complete the following information):

  • Device Hantek 6022BL
  • Program top line [e.g. *OpenHantek6022 (20220317 - commit 971) - Device DSO-6022BL (FW0210)
  • Input signal: ch2 has 0v to 3.3v 25% duty-cycle square wave at 3.613 kHz
  • Probe setting: probe 1 on constant signal, probe 2 on square wave
@ericfont
Copy link
Contributor Author

I can always get the crash...is completely reproducible.

@Ho-Ro
Copy link
Member

Ho-Ro commented Mar 25, 2022

Hi Eric,
I cannot reproduce the crash - never.
I see that you use Qt6, do you use the provided binary or did you build your own version?
To narrow down the issue please provide the trace output

OpenHantek --verbose=6 2> OH.log

(lot of output)

@Ho-Ro Ho-Ro added the BUG Something isn't working label Mar 25, 2022
@ericfont
Copy link
Contributor Author

log: OH.log

@ericfont
Copy link
Contributor Author

and it seems Qt version is actually 5.15.3:
image

Both my Qt 5 and Qt 6 builds are from arch linux repo.

@ericfont
Copy link
Contributor Author

I have edited my first comment to change Qt version to 5.15.3

@ericfont
Copy link
Contributor Author

The crash happens both when I use 32 Hz calibration square wave as well as the 100 kHz calibration square wave, as well as any frequency in between.

@ericfont
Copy link
Contributor Author

I'll build from latest git and debug in qtcreator to see what is going on.

@ericfont
Copy link
Contributor Author

here is the stack trace in Qt creator (with latest git main):

image

@ericfont
Copy link
Contributor Author

the index variable for iteration, k, has a suspiciously large value of 4294967295, so quite possible that maybe looking at some sortof overflow:

image

@ericfont
Copy link
Contributor Author

the samples vector only contains 20,000 doubles.

@ericfont
Copy link
Contributor Author

i is 0, because preTrigSamples is 0.

So I think I know how to reproduce it. You must put your trigger at the very left edge of the oscilloscope area.

@ericfont
Copy link
Contributor Author

so the bug happens when the blue trigger on top is all the way to the left like this:

image

But the bug doesn't happen when that blue trigger is placed to the right of the left edge.

In the loop k = i - 1 is used, which results in the maximum int!

@ericfont ericfont changed the title Crash when change "Trigger" "Slope" to "up&down" from just being up Crash when change "Trigger" "Slope" to "up&down" from just being up, if trigger set to left edge of screen Mar 25, 2022
@ericfont
Copy link
Contributor Author

ericfont commented Mar 25, 2022

the for loop condition tries to check for the condition that k is negative by using && k > 0, but unfortunately since k was declared as "unsigned", k will never be negative. So I think should remove unsigned.

@ericfont
Copy link
Contributor Author

(also looking at that for loop condition, and I see k >= i - swTriggerSampleSet, which would similarly be problematic if swTriggerSampleSet > i because that calculation would be treated as unsigned, thus resulting in a very large int result. So maybe put a (signed int) to force that calculation to be signed. Though that is not the issue causing this particular bug.)

@ericfont
Copy link
Contributor Author

I've made a simple PR that fixes this by using signed int for calculation, and casting all those unsigned ints to regular ints just to be safe incase there is another corner case such as swTriggerSampleSet > i.

@Ho-Ro Ho-Ro closed this as completed in a2c57e7 Mar 25, 2022
@Ho-Ro
Copy link
Member

Ho-Ro commented Mar 25, 2022

Cool, thank you for the detailed analysis of this error. I did also some more tests in the meantime and found also this k = i - 1 and solved it by refactoring - change all trigger position calculation variables from unsigned to int, because I do not like all this casting and int is big enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants