search

Qucs / qucsator

Circuit simulator of the Qucs project
http://qucs.sourceforge.net
GNU General Public License v2.0
23 stars 10 forks source link

fix unsafe vector access, which leads to crash with _GLIBCXX_ASSERTIO… #30

Closed ckoegler closed 2 years ago

ckoegler commented 2 years ago

This is a fix for issue #29 I reproduced the crash by compiling qucs with make CXXFLAGS='-D _GLIBCXX_ASSERTIONS -g'

I reproduced the crash with qucsator -i ~/.qucs/netlist.txt

then I did coredumpctl gdb qucsator and the command bt inside gdb to examine the core dump.

I used gdb together with the provided netlist gdb --args ./qucsator -i /netlist.txt to examine the function nodelist::assignNodes

felix-salfelder commented 2 years ago

On Wed, Jul 21, 2021 at 12:52:44PM -0700, Carsten Kögler wrote:

to examine the function nodelist::assignNodes

Well done. How about

?

Are there more of these? Perhaps _GLIBCXX_ASSERTIONS should be set in debug builds...

ckoegler commented 2 years ago
  • narray.resize(i+1);
  • narray[i] = n;
  • narray.push_back(n);

I thought it looked more intuitively. n gets written on index i. But I can change it.

Are there more of these?

Quite possibly. I thought we should look for them in the next step. But I don't know how. Inspect all usages of STL vectors? Just hope that we find them by running test-netlists?

Perhaps _GLIBCXX_ASSERTIONS should be set in debug builds...

What are debug builds and how do I make them? Including the flag is a good idea. We could also include it in the normal builds, but it maybe could cost performance.

felix-salfelder commented 2 years ago

On Thu, Jul 22, 2021 at 03:26:45PM -0700, Carsten Kögler wrote:

  • narray.resize(i+1);
  • narray[i] = n;
  • narray.push_back(n); I thought it looked more intuitively. n gets written on index i. But I can change it.

Nevermind.

Are there more of these? Quite possibly. I thought we should look for them in the next step. But I don't know how. Inspect all usages of ? Just hope that we find them by running test-netlists?

I agree. There are few tests included, I have never looked at them (or tried to use them). It would be useful to automate this (e.g. "make check").

Perhaps _GLIBCXX_ASSERTIONS should be set in debug builds... What are debug builds and how do I make them? Including the flag is a good idea. We could also include it in the normal builds, but it maybe could cost performance.

Production builds do -DNDEBUG -O2 (for speed), and debug builds have -g -O0. Iirc, The configure script takes an --enable-debug flag to control it. For some reason, _GLIBCXX_ASSERTIONS are not controlled by NDEBUG, likely because of the performance hit (some projects do not use NDEBUG at all). The flag should be set in debug builds only.

ckoegler commented 2 years ago

whoho. My first merged commit :-)

ckoegler commented 2 years ago

I agree. There are few tests included, I have never looked at them (or tried to use them). It would be useful to automate this (e.g. "make check").

You mean the stuff in qucsator\tests? I had a look. There are some "unit tests". But there seems not to exist some tests like in Qucs-test, e.g. input data (netlists in this case) vs. output data (qucs dataset file in this case).

How do we proceed? Make a 0.0.20-rc3 qucs release including the patched qucsator?

Production builds do -DNDEBUG -O2 (for speed), and debug builds have -g -O0. Iirc, The configure script takes an --enable-debug flag to control it. For some reason, _GLIBCXX_ASSERTIONS are not controlled by NDEBUG, likely because of the performance hit (some projects do not use NDEBUG at all). The flag should be set in debug builds only.

Should I make a pull request?

felix-salfelder commented 2 years ago

On Fri, Jul 23, 2021 at 04:52:35AM -0700, Carsten Kögler wrote:

e.g. input data (netlists in this case) vs. output data (qucs dataset file in this case).

Yes. I think end-to-end is safest. Qucsator prints too many digits, so some kind of numdiff is needed to check.

How do we proceed? Make a 0.0.20-rc2 qucs release including the patched qucsator?

That would be rc3. I'd rather not touch qucs 0.0.20-*, but feel free. I believe Qucsator is better off separately. One thing at a time.

Production builds do -DNDEBUG -O2 (for speed), and debug builds have -g -O0. Iirc, The configure script takes an --enable-debug flag to control it. For some reason, _GLIBCXX_ASSERTIONS are not controlled by NDEBUG, likely because of the performance hit (some projects do not use NDEBUG at all). The flag should be set in debug builds only.

Should I make a pull request?

Whatever makes sense. A section in README saying

$ ./configure --enable-debug CPPFLAGS=-D_GLIBCXX_ASSERTIONS $ make check

might do the trick in the end.