# Review of Music Software Code Defects\. Part 4\. Ardour

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\.

![0540_MusicSoftwareDefects_04_Ardour/image1.png](https://import.viva64.com/docx/blog/0540_MusicSoftwareDefects_04_Ardour/image1.png)

## Introduction

[Ardour](https://ardour.org/) 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](https://pvs-studio.com/en/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\. 

## 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\.

[V696](https://pvs-studio.com/en/docs/warnings/v696/) The 'continue' operator will terminate 'do \{ \.\.\. \} while \(FALSE\)' loop because the condition is always false\. Check lines: 394, 397\. session\_transport\.cc 394

```cpp
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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'strlen\(buf\) < 256' is always true\. vst\_info\_file\.cc 262

```cpp
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](https://pvs-studio.com/en/docs/warnings/v575/) The 'substr' function processes '\-1' elements\. Inspect the second argument\. meter\_strip\.cc 491

```cpp
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:

```cpp
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\.

## Something is Wrong in the Program

[V517](https://pvs-studio.com/en/docs/warnings/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

```cpp
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:

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

Another example:

[V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(working\_on\_selection\)' condition was already verified in line 284\. editor\_ops\.cc 314

```cpp
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\.

## 10 More Interesting Errors

**\#1**

[V512](https://pvs-studio.com/en/docs/warnings/v512/) A call of the 'memset' function will lead to underflow of the buffer 'error\_buffer'\. ardour\_http\.cc 142

```cpp
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:

* V512 A call of the 'memset' function will lead to underflow of the buffer 'error\_buffer'\. ardour\_http\.cc 208

**\#2**

[V541](https://pvs-studio.com/en/docs/warnings/v541/) It is dangerous to print the 'buf' string into itself\. luawindow\.cc 490

```cpp
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:

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

As a result we would like to receive a string: 

```cpp
N = 123, S = test
```

But in practice, we will have such a string in the buffer:

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v530/) The return value of function 'unique' is required to be utilized\. audio\_library\.cc 162

```cpp
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: 

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

**\#4**

[V654](https://pvs-studio.com/en/docs/warnings/v654/) The condition 'tries < 8' of loop is always true\. session\_transport\.cc 68

```cpp
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](https://pvs-studio.com/en/docs/warnings/v595/) The '\_session' pointer was utilized before it was verified against nullptr\. Check lines: 1576, 1579\. editor\_rulers\.cc 1576

```cpp
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:

* V595 The 'rui' pointer was utilized before it was verified against nullptr\. Check lines: 250, 253\. analysis\_window\.cc 250
* 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

**\#6**

[V614](https://pvs-studio.com/en/docs/warnings/v614/) Uninitialized variable 'req\.height' used\. Consider checking the second actual argument of the 'set\_size\_request' function\. time\_axis\_view\.cc 159

```cpp
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:

```cpp
void size_request(const Requisition& requisition);
```

The structure is passed by const reference and cannot be modified\.

**\#7**

[V746](https://pvs-studio.com/en/docs/warnings/v746/) Object slicing\. An exception should be caught by reference rather than by value\. ardour\_ui\.cc 3806

```cpp
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:

* V746 Object slicing\. An exception should be caught by reference rather than by value\. ardour\_ui\.cc 3670
* 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

**\#8**

[V762](https://pvs-studio.com/en/docs/warnings/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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'mootcher' pointer\. A memory leak is possible\. sfdb\_ui\.cc 1064

```cpp
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](https://pvs-studio.com/en/docs/warnings/v1002/) The 'XMLProcessorSelection' class, containing pointers, constructor and destructor, is copied by the automatically generated operator\=\. processor\_selection\.cc 25

```cpp
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_:

```cpp
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\. 

## 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\.

I would like to give an example of an interesting error from the directory _libs/vamp\-plugins_:

[V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. Transcription\.cpp 1827

```cpp
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\.

## 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\.

Other music software reviews:

* [Part 1\. MuseScore](https://pvs-studio.com/en/blog/posts/cpp/0530/)
* [Part 2\. Audacity](https://pvs-studio.com/en/blog/posts/cpp/0532/)
* [Part 3\. Rosegarden](https://pvs-studio.com/en/blog/posts/cpp/0536/)
* [Part 4\. Ardour](https://pvs-studio.com/en/blog/posts/cpp/0540/)
* [Part 5\. Steinberg SDKs](https://pvs-studio.com/en/blog/posts/cpp/0541/)

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](mailto:razmyslov@viva64.com)\.

It is very easy to try PVS\-Studio analyzer on your project, just go to the [download](https://pvs-studio.com/en/pvs-studio/download/) page\.