search

milkytracker / MilkyTracker

An FT2 compatible music tracker
http://milkytracker.github.io/
Other
1.68k stars 159 forks source link

Fix uninitialized memory being used #256

Closed nyanpasu64 closed 1 year ago

nyanpasu64 commented 2 years ago

I moved as much initialization as I could from constructor bodies into initializer lists, because I feel it's easier to track a single list of field initializers, than having to read two separate lists to see if a given field is initialized. I'm not sure if this was the best decision, because long initializer lists are ugly.

I ran MilkyTracker in ubsan, and found invalid bool accesses in these two structs. I initialized all fields I could find (though I might've missed some), instead of looking through control flow to see which fields were used or not. I don't like fields like PatternEditorControl::visibleWidth which are uninitialized until you call a method like PatternEditorControl::setSize() to set them, meaning the class isn't safe to use after the constructor, but only safe after calling methods.

I would very much prefer switching to C++11, which allows setting default values in the class declaration itself, so for most fields (which don't depend on the constructor), you don't have to match up the constructor with the class declaration to ensure all fields are initialized.

The result is clean in UBSAN, though I'm planning to perform more testing in valgrind, which is better at catching uninitialized memory usages than ASAN and UBSAN combined, to see if I missed any fields or any other classes with uninitialized memory.

See #251.

nyanpasu64 commented 2 years ago

Turns out valgrind catches uninitialized memory, and ASAN+UBSAN only catches invalid bools. MSAN claims to catch uninitialized memory, but I never tried since it requires rebuilding the C++ stdlib.

In any case, I've force-pushed with more UB fixes. This branch incorporates #255 though, since that also fixes UB.

coderofsalvation commented 1 year ago

nyanpasu64 were you able to look into the comments? Optimizations like this are very welcome..

nyanpasu64 commented 1 year ago

I can't look into the comments now. I never learned to use MilkyTracker, and haven't opened the program in months.

ghost commented 1 year ago

I can't look into the comments now. I never learned to use MilkyTracker, and haven't opened the program in months.

Got it, but I think the changes are fine.