You can’t just take and edit subtitles / PVS-Studio Blog

How many people use subtitles around the world? Probably a lot. For educational purposes or simply because of the love of the original voice acting, subtitles for almost any film and in many languages ​​can be found on the Internet. All this is created in special programs. As in most programs, Subtitle Edit could not do without surprises in the form of bugs.

Introduction

Subtitle Edit is a free editor with a huge list of features. This is a large open source C # project. The program is very popular and is issued in the first lines of the search engine results, many awards are listed on the project website. In the repository on GitHub, you can see that the project is actively developing, has many stars and forks. In general, this is a good project to participate in its development. Initially, I was just looking for a library for parsing subtitles, because most formats are not textual, but now I will return to my project a little later.

310 Issues are open on the project page on GitHub. Perhaps working with the results of the analysis will allow us to fix something. PVS-Studio Static Analyzer, which was used to study the code, issued 460 warnings (total for all levels of importance). Almost everything can and should be corrected. This is due to the fact that the analyzer has almost no recommended diagnostics. Found results usually signal real problems in the code. In the article I will give code examples, but I will choose only those errors that can greatly affect the work.

For more or less clear code snippets, I will send a Pull Request with corrections. But the author of the project is better to get acquainted with all the results of the analysis, checking the project yourself.

Ignoring styles

This is how a fragment of the form looks for setting the subtitle style:

And here is the analyzer warning for the code that is associated with this form:

V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 300, 302. SubStationAlphaStyles.cs 300

public static void AddStyle(ListView lv, SsaStyle ssaStyle,
  Subtitle subtitle, bool isSubstationAlpha)
{
  ....
  if (ssaStyle.Bold || ssaStyle.Italic)
    subItem.Font = new Font(...., FontStyle.Bold |
                                  FontStyle.Italic);
  else if (ssaStyle.Bold)
    subItem.Font = new Font(...., FontStyle.Bold);
  else if (ssaStyle.Italic)
    subItem.Font = new Font(...., FontStyle.Italic);
  else if (ssaStyle.Italic)
    subItem.Font = new Font(...., FontStyle.Regular);
  ....
}

In total, the analyzer issued 4 warnings for this code fragment. And this is not surprising, because there is an error in almost every line. Moreover, the option with ssaStyle.Underline is not considered here !

It’s better to rewrite the code to something like this and do it very carefully:

....
if (ssaStyle.Bold)
  fontStyles |= FontStyle.Bold;
....
subItem.Font = new Font(...., fontStyles);
....

The last paragraph of text is not deleted

V3022 CWE-570 Expression '_networkSession! = Null && _networkSession.LastSubtitle! = Null && i <_networkSession.LastSubtitle.Paragraphs.Count' is always false. Main.cs 7242

private void DeleteSelectedLines()
{
  ....
  if (_networkSession != null)                // <=
  {
    _networkSession.TimerStop();
    NetworkGetSendUpdates(indices, 0, null);
  }
  else
  {
    indices.Reverse();
    foreach (int i in indices)
    {
      _subtitle.Paragraphs.RemoveAt(i);
      if (_networkSession != null &&          // <=
          _networkSession.LastSubtitle != null &&
          i < _networkSession.LastSubtitle.Paragraphs.Count)
        _networkSession.LastSubtitle.Paragraphs.RemoveAt(i);
    }
  ....
  }
  ....
}

The _networkSession variable is already checked in the first condition, therefore, in the else branch it is guaranteed to be null . This combination of checks led to a false condition and unreachable code.

Loss of functionality due to typos

V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 113, 115. SsaStyle.cs 113

public string ToRawSsa(string styleFormat)
{
  var sb = new StringBuilder();
  sb.Append("Style: ");
  var format = ....;
  for (int i = 0; i < format.Length; i++)
  {
    string f = format[i].Trim();
    if (f == "name")
      sb.Append(Name);
    ....
    else if (f == "shadow")    // <=
      sb.Append(OutlineWidth); // <=
    else if (f == "shadow")    // <=
      sb.Append(ShadowWidth);  // <=
    ....
  }
  ....
}

Typos in a cascade of conditions result in unreachable code branches. Often this code is the result of Copy-Paste programming. In the above example, the second duplicated condition will never be fulfilled. And this is the simplest and most compact example that I have chosen for the article. A lot of such examples were found in the project to describe the problem in a separate section.

Here is the entire list of Copy-Paste code that needs to be fixed:

• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 268, 270. ExportCustomTextFormat.cs 268
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 278, 280. ExportCustomTextFormat.cs 278
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 220, 252. SetSyncPoint.cs 220
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 712, 743. ExportPngXml.cs 712
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 712, 743. ExportPngXml.cs 712
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 162, 178. LambdaCap.cs 162
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 166, 182. LambdaCap.cs 166
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 170, 186. LambdaCap.cs 170
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 174, 190. LambdaCap.cs 174
• V3003 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 398, 406. Ebu.cs 398
• V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless FinalCutProTest2Xml.cs 22
• V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless FinalCutProTextXml.cs 21
• V3021 CWE-561 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless FinalCutProXml.cs 22

Something is wrong with a 720x480 picture

V3022 CWE-570 Expression 'param.Bitmap.Width == 720 && param.Bitmap.Width == 480' is always false. Probably the '||' operator should be used here. ExportPngXml.cs 1808

private static string FormatFabTime(TimeCode time,
                                    MakeBitmapParameter param)
{
  if (param.Bitmap.Width == 720 && param.Bitmap.Width == 480)
    return $"....";
  // drop frame
  if (Math.Abs(param.... - 24 * (999 / 1000)) < 0.01 ||
      Math.Abs(param.... - 29 * (999 / 1000)) < 0.01 ||
      Math.Abs(param.... - 59 * (999 / 1000)) < 0.01)
      return $"....";
  return $"....";
}

The confusion with Width and Height is a classic typo example. But something else is suspicious in this function. All abbreviations of the line that I replaced with four dots are the same line: {time.Hours: 00}; {time.Minutes: 00}; {time.Seconds: 00}; {SubtitleFormat.MillisecondsToFramesMaxFrameRate (time.Milliseconds) : 00} . Those. the presence of two conditions does not affect the result of the function, the function always returns the same thing.

Matryoshka loading is always successful

V3009 CWE-393 It's odd that this method always returns one and the same value of 'true'. Main.cs 10153

private bool LoadTextSTFromMatroska(
  MatroskaTrackInfo matroskaSubtitleInfo,
  MatroskaFile matroska,
  bool batchMode)
{
  ....
  _fileDateTime = new DateTime();
  _converted = true;
  if (batchMode)
      return true;
  SubtitleListview1.Fill(_subtitle, _subtitleAlternate);
  if (_subtitle.Paragraphs.Count > 0)
      SubtitleListview1.SelectIndexAndEnsureVisible(0);
  ShowSource();
  return true;
}

A function was found that always returns true . This is probably a mistake. The value of this function is checked in four places in the program. Also nearby are similar functions in the code, for example, LoadDvbFromMatroska (), and it returns different values.

Useless or erroneous code

V3022 CWE-571 Expression 'listBoxVobFiles.Items.Count> 0' is always true. DvdSubRip.cs 533

private void DvdSubRip_Shown(object sender, EventArgs e)
{
  if (string.IsNullOrEmpty(_initialFileName))
    return;
  if (_initialFileName.EndsWith(".ifo", ....))
  {
    OpenIfoFile(_initialFileName);
  }
  else if (_initialFileName.EndsWith(".vob", ....))
  {
    listBoxVobFiles.Items.Add(_initialFileName);
    buttonStartRipping.Enabled = listBoxVobFiles.Items.Count > 0;
  }
  _initialFileName = null;
}

An item is added to the listBoxVobFiles list , and then it is checked to see if the list is empty. Obviously, there will be at least one element. And there are more than thirty such checks, which are always true or false, in the project.

Just a fun example.

V3005 The 'positionInfo' variable is assigned to itself. WebVTT.cs 79

internal static string GetPositionInfoFromAssTag(Paragraph p)
{
  ....
  if (!string.IsNullOrEmpty(line))
  {
    if (positionInfo == null)
      positionInfo = " line:" + line;
    else
      positionInfo = positionInfo += " line:" + line;
  }
  ....
}

Choosing between the recording options “A = A + n” and “A + = n”, the author of this code chose a compromise option “A = A + = n”: D

Conclusion

To understand how to fix a particular analyzer warning, you need to understand a little about the subtitle formats and the features of their processing. Therefore, if there are people who want to support the project and leave Pull Requests with corrections to the author of the project on GitHub , then here is the link to download the HTML report with PVS-Studio warnings of High / Medium level.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. One Doesn't Simply Edit Subtitles