Review of music software code defects. Part 4. Ardour

Ardour is still the largest of the musical projects involved in the review of code defects. The project includes about 1000 source code files in C ++. The project is actively supported by the developer community, and I did not find any mention of using any static analysis tools. As a result - a lot of errors of different nature. The article will describe the most interesting of them.

Introduction

Ardour - digital audio workstation (Digital Audio Workstation). Works on Linux, Mac OS X and FreeBSD. Ardour’s capabilities are limited only by the hardware on which it is running. This makes the program one of the most popular tools for working with sound in a professional environment.

Ardour uses many third-party libraries. Some of them are located with the source code Ardour and edited by its authors. Also, the project is divided into different components. The article includes only the most interesting errors from the gtk2_ardour and libs / ardour directories . To view the full report, authors can verify the project themselves by requesting a temporary key in support.

The analysis was performed using PVS-Studio . It is a tool for detecting errors in the source code of programs written in C, C ++ and C # languages. Works in the environment of Windows and Linux.
')

What is the author's idea?

In this section I will give a few examples of code according to which the opinions of readers can be divided: an error or a false positive. And the correct solution is to rewrite the code anyway, so that it does not confuse other developers and analysis tools.

V696 The 'continue' operator will terminate 'do {...} while (FALSE)' loop because the condition is always false. Check lines: 394, 397. session_transport.cc 394

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); .... } 

The do-while (false) loop can be used together with the continue statement to go to the end of the block (an analogue of goto ), but why then is the break statement? It is possible that an error was made in the code and the loop should be do-while (true) . In general, the code can and should be rewritten.

Note. Perhaps not everyone understood the point, so I will explain in a little more detail. The continue operator transfers control not to the beginning of the do-while operator, but to the condition. Since the condition is always false, here the continue operator works in exactly the same way as the break statement.

V547 Expression 'strlen (buf) <256' is always true. vst_info_file.cc 262

 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; } } 

The fgets () function takes as its second argument the maximum length of the string, including the terminal zero, i.e. the buf buffer will end with a null character correctly. And what happens next in the code? The condition (strlen (buf) <MAX_STRING_LEN) is always true, since The fgets () function reads no more than (MAX_STRING_LEN - 1) characters. Further, if the string is not empty, then the last character is deleted from it. Not sure what this programmer planned to write. Most likely, he expected that the string is not limited to the null character, but then such a string cannot be passed to the strlen () function. In general, the code should be rewritten so that you do not have to guess how it works and whether it corresponds to the originally intended one.

V575 The 'substr' function processes '-1' elements. Inspect the second argument. meter_strip.cc 491

 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)); // <= } } } 

Note all calls to the substr () function. The second argument is the value -1 . But what does it mean? Here is the function prototype:

 string substr (size_t pos = 0, size_t len = npos) const; 

According to the documentation, without the 2nd argument, the substr () function returns a substring from the specified position to the end of the string. Those. instead of simply writing substr (pos) or at least substr (pos, string :: npos) , the programmer decided to pass a value of -1 , which eventually implicitly converts to the size_t type and turns into the value string :: npos . Probably, the code is correct, but ugly. In general, this can and should be rewritten.

Something is wrong in the program.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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(); } } 

Due to the same conditional expressions, the update_track_number_visibility () function is never called. It looks like the track number is not updated correctly at the right moment.

Another five suspicious places:

• V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 160, 170. event_type_map.cc 160
• V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 possibility 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 possibility 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 possibility of logical error presence. Check lines: 477, 519. ardour_ui_options.cc 477

Another example:

V571 Recurring check. The 'if (working_on_selection)' condition was already verified in line 284. editor_ops.cc 314

 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 (); } 

The logical variable working_on_selection is checked a second time, in such a way that the condition is always false. Perhaps due to such an error, some interface element is incorrectly highlighted.

10 more interesting mistakes

#one

V512 A call to the 'memset' function will lead to the underflow of the buffer 'error_buffer'. ardour_http.cc 142

 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)); .... } 

I often encountered errors when, for example, not the size of an object, but the size of a pointer to it is passed to the memset () function, but something new was found here. Instead of the entire array, only one byte is reset.

Another such place:

• V512 A call to the 'memset' function will lead to the underflow of the buffer 'error_buffer'. ardour_http.cc 208

# 2

V541 It is dangerous to print the 'buf' string into itself. luawindow.cc 490

 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? .... } 

Here form a certain line in the buffer. Then they want to get a new line, keeping the previous value of the line and adding the value of the function random () to it . It seems simple.

In the code, the original comment of the developer, who doubted the correctness of the code, was left. To explain why an unexpected result can be obtained here, I will quote a simple and understandable example from the documentation for this diagnostic:

 char s[100] = "test"; sprintf(s, "N = %d, S = %s", 123, s); 

As a result of this code, I want to get the line:

 N = 123, S = test 

But in practice, the string will be formed in the buffer:

 N = 123, S = N = 123, S = 

In other situations, a similar code can lead not only to the output of incorrect text, but also to the program crash. The code can be corrected by using a new buffer to save the result. Correct option:

 char s1[100] = "test"; char s2[100]; sprintf(s2, "N = %d, S = %s", 123, s1); 

In the case of the control line "% s% ld" troubles may not occur, and the correct line will be printed. But the code is very dangerous and unreliable.

# 3

V530 audio_library.cc 162

 void AudioLibrary::search_members_and (vector<string>& members, const vector<string>& tags) { .... sort(members.begin(), members.end()); unique(members.begin(), members.end()); .... } 

Removing duplicate elements from the members vector is done incorrectly. After calling the unique () function, there are undefined elements in the vector.

Corrected code:

 sort(members.begin(), members.end()); auto last = unique(members.begin(), members.end()); v.erase(last, members.end()); 

#four

V654 The condition 'tries <8' of loop is always true. session_transport.cc 68

 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; } 

The code above assumes 8 attempts of a certain operation, but the tries variable counter does not change in a loop. Consequently, there is only one exit point from the cycle and, judging by the commentary, it indicates successful execution. Because of such a defect in the code, the program conceals potential errors and may hang during execution.

#five

V595 The '_session' pointer was used before it was verified against nullptr. Check lines: 1576, 1579. editor_rulers.cc 1576

 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; } .... } 

This place looks like a serious mistake. If the _session field is zero, then dereferencing of a non-valid pointer will occur before the corresponding check.

Another small list of similar places:

• V595 The 'rui' pointer was used before it was verified nullptr. Check lines: 250, 253. analysis_window.cc 250
• V595 The 'scan_dlg' pointer was used before it was verified against nullptr. Check lines: 5089, 5099. ardour_ui.cc 5089
• V595 The '_session' pointer was used before it was verified against nullptr. Check lines: 352, 361. ardour_ui_options.cc 352
• It was verified against nullptr. Check lines: 581, 586. editor_mouse.cc 581
• V595 The '_a_window' pointer was used before it was counted against nullptr. Check lines: 423, 430. fft_graph.cc 423
• V595 The '_editor -> _ session' pointer was used before it was verified against nullptr. Check lines: 140, 142. verbose_cursor.cc 140

# 6

V614 Uninitialized variable 'req.height' used. Consider the set_size_request function. time_axis_view.cc 159

 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); .... } 

By this example, it was not immediately clear why the req structure is not initialized. But looking at the sources and documentation, I found a function prototype:

 void size_request(const Requisition& requisition); 

The structure is transmitted by a constant link and can not be changed.

# 7

V746 Object slicing. By reference rather than by value. ardour_ui.cc 3806

 int ARDOUR_UI::build_session (....) { .... try { new_session = new Session (....); } catch (SessionException e) { .... return -1; } catch (...) { .... return -1; } .... } 

The analyzer has detected a potential error related to catching an exception by value. This means that with the help of the copy constructor, a new e object of the SessionException type will be constructed. In this case, some of the exception information that was stored in classes inherited from SessionException will be lost. More correctly and, in addition, more effectively, intercept an exception by reference.

Other warnings of this type:

• V746 Object slicing. By reference rather than by value. ardour_ui.cc 3670
• V746 Object slicing. By reference rather than by value. luawindow.cc 467
• V746 Object slicing. By reference rather than by value. luawindow.cc 518
• V746 Object slicing. By reference rather than by value. luainstance.cc 1326
• V746 Object slicing. By reference rather than by value. luainstance.cc 1363

#eight

V762 It was possible a virtual function was overridden incorrectly. See the second argument of the function 'set_mouse_mode' in the derived class 'Editor' and the base class 'PublicEditor'. editor.h 184

 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); .... } 

Immediately, two functions in the Editor class were redefined incorrectly. You can't just take the default value of an argument like this :).

#9

V773 The function of the mootcher pointer. A memory leak is possible. sfdb_ui.cc 1064

 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 ""; } 

The mootcher pointer is released on one condition. In other cases, a memory leak occurs.

#ten

V1002 The 'XMLProcessorSelection' class, containing pointers, constructor and destructor, is copied by the automatically generated operator =. processor_selection.cc 25

 XMLProcessorSelection processors; ProcessorSelection& ProcessorSelection::operator= (ProcessorSelection const & other) { if (this != &other) { processors = other.processors; } return *this; } 

One of the new diagnostics of PVS-Studio has found an interesting error. Assigning one object of the XMLProcessorSelection class to another causes the pointer inside these objects to refer to the same memory location.

XMLProcessorSelection class definition :

 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; // <= }; 

As we can see, the class contains a node pointer, but does not have an overriding assignment operator. Most likely, instead of assignment, it was necessary to use the set () or add () functions.

Where else can you look for errors?

Articles always include a limited number of error examples. Also in this review I took examples only from the gtk2_ardour and libs / ardour directories. But there are a lot of sources in the Ardore project, and by studying all the analysis results, you can significantly improve both the quality of the project code and the stability of the program.

I will give one example of an interesting error from the libs / vamp-plugins directory:

V523 The 'then' statement is equivalent to the 'else' statement. Transcription.cpp 1827

 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]; } } .... } 

The analyzer detected the same branches of the conditional operator. The fact that the condition checks whether the element is positive or not makes this code fragment even more suspicious.

Conclusion

The Ardour project is likely to be more in demand in a professional environment than previous projects from the review. Therefore, there should be quite a lot of people interested in correcting errors.

Other reviews:

• Review of music software code defects. Part 1. MuseScore
• Review of music software code defects. Part 2. Audacity
• Review of music software code defects. Part 3. Rosegarden

If you know an interesting software for working with music and want to see it in the review, then send the names to me by mail .

It is very easy to try the PVS-Studio analyzer on your project, just go to the download page.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Review of Music Software Code Defects Part 4. Ardour