Ardour is so far the largest of musical projects involved in the review of code defects. The project includes about 1000 files of source code in C++. The project is actively maintained by the community of developers, but at the same time I did not find mentions of any applied static analysis tools. As a result, there are many different types of errors. The article will describe the most interesting ones.
Ardour is a Digital Audio Workstation. It runs on Linux, macOS 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.
Ardour uses many third-party libraries. Some of them are located with the Ardour source code and are edited by its authors. The project is divided into different components. The article includes only the most interesting errors from the directories gtk2_ardour and libs/ardour. To view the full report, authors can independently check the project, having sent a request for a temporary key to our support.
Analysis was performed using PVS-Studio. PVS-Studio is a tool for bug detection in the source code of programs, written in C, C++, and C#. It works in Windows and Linux environment.
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.
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);
....
}A do-while(false) loop can be used jointly by the continue operator to go to the end of the block (goto analog), but why is the break operator here? Perhaps, a mistake was made in the code and the loop must be do-while(true). So, the code can and should be rewritten.
Note. Perhaps, not all readers understood the main point, so let me explain in more detail. The operator continue passes the control not to the beginning of a do-while operator, but to a condition. As the condition is always false, here the operator continue works in exactly the same way as the operator break.
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 function fgets() takes the maximum string length, including terminal null as the second argument, i.e. the buf buffer will null-fail correctly. What happens next in the code? The (strlen (buf) < MAX_STRING_LEN) condition is always true, because the function fgets() reads no more than (MAX_STRING_LEN-1) characters. Further, if the string is not empty, then the last character is removed from it. I'm not sure that this is what a developer was planning to write. Most likely, he expected that the line has not been limited by the null character, but in this case, this line cannot be passed to the strlen() function. In general, code should be rewritten, so that you don't have to guess how it works and whether it complies with the originally intended idea.
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)); // <=
}
}
}Please, pay attention to all calls to the function substr(). The second argument passes the value -1. But what does it mean? The function prototype looks as follows:
string substr (size_t pos = 0, size_t len = npos) const;According to the documentation, without the 2nd argument function substr() returns the substring from the specified position to the end of the line. So, instead of simply writing substr(pos) or at least substr (pos, string::NPOs), a developer has decided to pass the value -1, which ultimately is implicitly converted to the type size_t and turns into the value string::npos. Probably, the code is correct but it doesn't look nice. So, it can and should be rewritten.
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();
}
}Because of the same conditional expressions, the function update_track_number_visibility() is never called. It looks like the track number is not updated correctly at the right moment.
Five more suspicious fragments:
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 ();
}A boolean variable working_on_selection is checked the second time, so that the condition will always be false. Perhaps, due to an error, some UI element is selected incorrectly.
#1
V512 A call of the 'memset' function will lead to 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 came across errors when developers, for instance, pass to function memset() not the size of the object, but the size of the pointer on it. Here I found something new. Instead of a whole array they would nullify only one byte.
One more similar fragment:
#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?
....
}A string is formed in the buffer. Then a developer wants to get a new string, having saved the previous string value, and having added the value of the function random() to it. It seems really simple.
There is the original comment in code, left by a developer, who doubted in code correctness. To explain why an unexpected result may be received here, I will quote a simple and clear example from the documentation for this diagnostic:
char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);As a result we would like to receive a string:
N = 123, S = testBut in practice, we will have such a string in the buffer:
N = 123, S = N = 123, S =In other situations, the same code can lead not only to the incorrect text, but also to the program abortion. The code can be fixed if you use a new buffer to store the result. The correct version:
char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1);In case of the "%s%ld" control string, the problem might not occur and the correct string will be generated. But the code is very dangerous and insecure.
#3
V530 The return value of function 'unique' is required to be utilized. 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());
....
}A removal of duplicated elements from a members vector was written incorrectly. After calling the function unique() the undefined elements remain in vector.
Correct variant of the code:
sort(members.begin(), members.end());
auto last = unique(members.begin(), members.end());
v.erase(last, members.end());#4
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 above code assumes 8 attempts of some operation but the counter variable tries does not change in the loop. Therefore, there is only one exit point from the loop and, judging by the comment, it testifies of a successful performance. Due to this defect in code, a hiding of potential errors occurs in the program and possible lockups are possible when running.
#5
V595 The '_session' pointer was utilized 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 feels like a serious error. If the field _session is null, a dereferencing of invalid pointer will happen before the appropriate verification.
A list of similar fragments:
#6
V614 Uninitialized variable 'req.height' used. Consider checking the second actual argument of 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);
....
}In this example, it was not immediately clear why the structure req was not initialized. But having looked at the source code and documentation, I found a function prototype:
void size_request(const Requisition& requisition);The structure is passed by const reference and cannot be modified.
#7
V746 Object slicing. An exception should be caught 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 detected a potential error, related to catching the exception by value. It means that a new e object of SessionException type will be constructed using a copy constructor. At the same time, some information about the exception that was stored in the classes, inherited from TSystemException will be lost. It is more correct and, in addition, more effective to catch an exception by reference.
Other warnings of this type:
#8
V762 It is possible a virtual function was overridden incorrectly. See second argument of function 'set_mouse_mode' in derived class 'Editor' and 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);
....
}At once two functions in the class Editor have been overridden incorrectly. One does not simply change the default argument value :).
#9
V773 The function was exited without releasing 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 pointer mootcher is released under one condition. In other cases, a memory leak occurs.
#10
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 bug. Assigning one object of the class XMLProcessorSelection to another, causes the situation when the pointer inside these objects refers to the same memory area.
Definition of the class XMLProcessorSelection:
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 it does not have the overridden assignment operator. Most likely, instead of assignment, set() or add() functions had to be used.
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.
I would like to give an example of an interesting error from the directory libs/vamp-plugins:
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 has detected the similar branches of a conditional operator. The fact that a check is performed in the condition, regardless whether the item is positive or not, makes this fragment of code even more suspicious.
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.
Other music software reviews:
If you know an interesting soft to work with music and want to see it in review, then send me the names of the programs by mail.
It is very easy to try PVS-Studio analyzer on your project, just go to the download page.