Review of Music Software Code Defects Part 4. Ardour

Introduction

Ardour is a Digital Audio Workstation. It runs on Linux, Mac OS X and FreeBSD. Ardor’s functionality is limited only by the equipment, on which it is running. This makes the program one of the most popular tools for working with sound in professional environment.

What Is the Author’s Idea?

In this section I will give some code examples in which the readers’ opinions might divide if it is an error or a false positive. The right solution is to rewrite the code anyway, so that it would not confuse other developers and analysis tools.

void
Session::butler_transport_work ()
{
....
do {
more_disk_io_to_do = _butler->flush_tracks_to_disk_after_....
if (errors) {
break;
}
if (more_disk_io_to_do) {
continue;
}
} while (false);
....
}
static char *
read_string (FILE *fp)
{
char buf[MAX_STRING_LEN];
if (!fgets (buf, MAX_STRING_LEN, fp)) {
return 0;
}
if (strlen (buf) < MAX_STRING_LEN) {
if (strlen (buf)) {
buf[strlen (buf)-1] = 0;
}
return strdup (buf);
} else {
return 0;
}
}
void
MeterStrip::set_tick_bar (int m)
{
std::string n;
_tick_bar = m;
if (_tick_bar & 1) {
n = meter_ticks1_area.get_name();
if (n.substr(0,3) != "Bar") {
meter_ticks1_area.set_name("Bar" + n);
}
} else {
n = meter_ticks1_area.get_name();
if (n.substr(0,3) == "Bar") {
meter_ticks1_area.set_name(n.substr(3,-1)); // <=
}
}
if (_tick_bar & 2) {
n = meter_ticks2_area.get_name();
if (n.substr(0,3) != "Bar") {
meter_ticks2_area.set_name("Bar" + n);
}
} else {
n = meter_ticks2_area.get_name();
if (n.substr(0,3) == "Bar") {
meter_ticks2_area.set_name(n.substr(3,-1)); // <=
}
}
}
string substr (size_t pos = 0, size_t len = npos) const;

Something is Wrong in the Program

V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 2389, 2409. mixer_strip.cc 2389

void
MixerStrip::parameter_changed (string p)
{
if (p == _visibility.get_state_name()) {
....
} else if (p == "track-name-number") { // <=
name_changed ();
} else if (p == "use-monitor-bus") {
....
} else if (p == "track-name-number") { // <=
update_track_number_visibility();
}
}
  • V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 4065, 4151. session_state.cc 4065
  • V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 4063, 4144. session_state.cc 4063
  • V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 498, 517. ardour_ui_options.cc 498
  • V517 The use of ‘if (A) {…} else if (A) {…}’ pattern was detected. There is a probability of logical error presence. Check lines: 477, 519. ardour_ui_options.cc 477
void
Editor::split_regions_at (....)
{
....
if (working_on_selection) {
....
} else {
if( working_on_selection ) {
//these are the new regions created after the split
selection->add (latest_regionviews);
}
}
commit_reversible_command ();
}

10 More Interesting Errors

#1

class HttpGet {
....
char error_buffer[CURL_ERROR_SIZE];
....
};
HttpGet::HttpGet (bool p, bool ssl)
: persist (p)
, _status (-1)
, _result (-1)
{
memset (error_buffer, 0, sizeof (*error_buffer));
....
}
void
LuaWindow::save_script ()
{
....
do {
char buf[80];
time_t t = time(0);
struct tm * timeinfo = localtime (&t);
strftime (buf, sizeof(buf), "%s%d", timeinfo);
sprintf (buf, "%s%ld", buf, random ()); // is this valid?
....
}
char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);
N = 123, S = test
N = 123, S = N = 123, S =
char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1);
void
AudioLibrary::search_members_and (vector<string>& members,
const vector<string>& tags)
{
....
sort(members.begin(), members.end());
unique(members.begin(), members.end());
....
}
sort(members.begin(), members.end());
auto last = unique(members.begin(), members.end());
v.erase(last, members.end());
void
Session::add_post_transport_work (PostTransportWork ptw)
{
PostTransportWork oldval;
PostTransportWork newval;
int tries = 0;
while (tries < 8) {
oldval = (PostTransportWork) g_atomic_int_get (....);
newval = PostTransportWork (oldval | ptw);
if (g_atomic_int_compare_and_exchange (....)) {
/* success */
return;
}
}
error << "Could not set post transport work! ...." << endmsg;
}
void
Editor::set_minsec_ruler_scale (samplepos_t lower,
samplepos_t upper)
{
samplepos_t fr = _session->sample_rate() * 1000;
samplepos_t spacer;
if (_session == 0) {
return;
}
....
}
  • V595 The ‘scan_dlg’ pointer was utilized before it was verified against nullptr. Check lines: 5089, 5099. ardour_ui.cc 5089
  • V595 The ‘_session’ pointer was utilized before it was verified against nullptr. Check lines: 352, 361. ardour_ui_options.cc 352
  • V595 The ‘al’ pointer was utilized before it was verified against nullptr. Check lines: 581, 586. editor_mouse.cc 581
  • V595 The ‘_a_window’ pointer was utilized before it was verified against nullptr. Check lines: 423, 430. fft_graph.cc 423
  • V595 The ‘_editor->_session’ pointer was utilized before it was verified against nullptr. Check lines: 140, 142. verbose_cursor.cc 140
TimeAxisView::TimeAxisView (....)
{
....
boost::scoped_ptr<Gtk::Entry> an_entry (new FocusEntry);
an_entry->set_name (X_("TrackNameEditor"));
Gtk::Requisition req;
an_entry->size_request (req);
name_label.set_size_request (-1, req.height);
name_label.set_ellipsize (Pango::ELLIPSIZE_MIDDLE);
....
}
void size_request(const Requisition& requisition);
int
ARDOUR_UI::build_session (....)
{
....
try {
new_session = new Session (....);
}
catch (SessionException e) {
....
return -1;
}
catch (...) {
....
return -1;
}
....
}
  • V746 Object slicing. An exception should be caught by reference rather than by value. luawindow.cc 467
  • V746 Object slicing. An exception should be caught by reference rather than by value. luawindow.cc 518
  • V746 Object slicing. An exception should be caught by reference rather than by value. luainstance.cc 1326
  • V746 Object slicing. An exception should be caught by reference rather than by value. luainstance.cc 1363
class PublicEditor : ....
{
....
virtual void
set_mouse_mode (Editing::MouseMode m, bool force = false) = 0;
virtual void
set_follow_playhead (bool yn, bool catch_up = false) = 0;
....
}
class Editor : public PublicEditor, ....
{
....
void set_mouse_mode (Editing::MouseMode, bool force=true);
void set_follow_playhead (bool yn, bool catch_up = true);
....
}
std::string
SoundFileBrowser::freesound_get_audio_file(Gtk::TreeIter iter)
{
Mootcher *mootcher = new Mootcher;
std::string file;
string id = (*iter)[freesound_list_columns.id];
string uri = (*iter)[freesound_list_columns.uri];
string ofn = (*iter)[freesound_list_columns.filename];
if (mootcher->checkAudioFile(ofn, id)) {
// file already exists, no need to download it again
file = mootcher->audioFileName;
delete mootcher;
(*iter)[freesound_list_columns.started] = false;
return file;
}
if (!(*iter)[freesound_list_columns.started]) {
// start downloading the sound file
(*iter)[freesound_list_columns.started] = true;
mootcher->fetchAudioFile(ofn, id, uri, this);
}
return "";
}
XMLProcessorSelection processors;ProcessorSelection&
ProcessorSelection::operator= (ProcessorSelection const & other)
{
if (this != &other) {
processors = other.processors;
}
return *this;
}
class XMLProcessorSelection {
public:
XMLProcessorSelection() : node (0) {}
~XMLProcessorSelection() { if (node) { delete node; } }
void set (XMLNode* n) {
if (node) {
delete node;
}
node = n;
}
void add (XMLNode* newchild) {
if (!node) {
node = new XMLNode ("add");
}
node->add_child_nocopy (*newchild);
}
void clear () {
if (node) {
delete node;
node = 0;
}
}
bool empty () const { return node == 0 || ....empty(); } const XMLNode& get_node() const { return *node; } private:
XMLNode* node; // <=
};

Where Else Can You Search for Errors?

Articles always include a limited number of examples of errors. Also, in this review I took the examples only from the directories gtk2_ardour and libs/ardour.Nevertheless, ther are a lot of sources in Ardore project, and when examining all analysis results you can greatly improve both the project code quality and the stability of the program work.

void Transcribe(....)
{
....
for (j=0;j<112;j++)
{
....
if(A1[j]>0)
{
D[j]=A1[j];D2[j]=A1[j];
}
else
{
D[j]=A1[j];D2[j]=A1[j];
}
}
....
}

Conclusion

The Ardour project is probably more popular in professional environment than the previous projects of the review. Therefore, there might be many people interested in its bugs fixing.

--

--

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

610 Followers

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.