Emby is quite a popular media server along with Plex and Kodi. In this article, we'll discuss the bugs found in its source code with the static analyzer PVS-Studio. The remark "Built with ReSharper" on the project's official website makes the analysis even more interesting.
PVS-Studio runs on 64-bit Windows, Linux, and macOS systems. It can detect bugs in the source code of software written in C, C++, C#, and Java.
Emby is a media server; its source code is available on GitHub. It allows the user to stream and access their media content (video, audio, photos) on any device. Here's a brief summary of Emby's features according to the project's official website:
PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'c != '<'' to the left and to the right of the '&&' operator. HttpListenerRequest.Managed.cs 49
internal void SetRequestLine(string req)
{
....
if ((ic >= 'A' && ic <= 'Z') ||
(ic > 32 && c < 127 && c != '(' && c != ')' && c != '<' && // <=
c != '<' && c != '>' && c != '@' && c != ',' && c != ';' && // <=
c != ':' && c != '\\' && c != '"' && c != '/' && c != '[' &&
c != ']' && c != '?' && c != '=' && c != '{' && c != '}'))
continue;
....
}The analyzer has detected a duplicate subexpression c != '<'. One explanation is that this is a programming mistake and the developer meant to write something else in place of '<'. Another, more likely explanation, is that the second subexpression wasn't meant to be there at all and should be removed.
PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'SmbConstants.AttrHidden' to the left and to the right of the '|' operator. SmbComDelete.cs 29
internal SmbComDelete(string fileName)
{
Path = fileName;
Command = SmbComDelete;
_searchAttributes = SmbConstants.AttrHidden |
SmbConstants.AttrHidden |
SmbConstants.AttrSystem;
}Another typo that has to do with duplicate subexpressions. As a side note, there are too many issues like that in Emby's source code – mistakes caused by inattention. I'm not blaming the developers though; we can all be absent-minded at times (examples, examples, examples), and this is exactly why static analysis exists – to protect us from our own mistakes.
PVS-Studio diagnostic message: V3004 The 'then' statement is equivalent to the 'else' statement. SqliteItemRepository.cs 5648
private QueryResult<Tuple<BaseItem, ItemCounts>> GetItemValues(....)
{
....
if (typesToCount.Length == 0)
{
whereText += " And CleanName In (Select CleanValue
from ItemValues where "
+ typeClause + " AND ItemId in (select guid from TypedBaseItems"
+ innerWhereText + "))";
}
else
{
//whereText += " And itemTypes not null";
whereText += " And CleanName In (Select CleanValue
from ItemValues where "
+ typeClause + " AND ItemId in (select guid from TypedBaseItems"
+ innerWhereText + "))";
}
....
}And this one looks very much like a copy-paste bug because the if and else blocks have the same bodies. What's the good of checking the typesToCount array's size if it doesn't affect the subsequent execution logic? This is something that only the authors know.
PVS-Studio diagnostic message: V3005 The '_validProviderIds' variable is assigned to itself. BaseNfoParser.cs 77
private Dictionary<string, string> _validProviderIds;
....
public void Fetch(....)
{
....
_validProviderIds = _validProviderIds = new Dictionary<....>(....);
....
}Another typo, which results in assigning a variable its own value. This code needs revising.
PVS-Studio diagnostic message: V3008 The 'Chapters' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 29, 28. Title.cs 29
public Title(uint titleNum)
{
ProgramChains = new List<ProgramChain>();
Chapters = new List<Chapter>();
Chapters = new List<Chapter>();
TitleNumber = titleNum;
}It's about inattention and typos again... The Chapters variables are assigned a value twice. Sure, this duplicate assignment isn't going to do any harm, but you still don't want things like that in your code. No point lingering on this one, so let's move on.
PVS-Studio diagnostic message: V3013 It is odd that the body of 'Read' function is fully equivalent to the body of 'Write' function (407, line 415). BaseSqliteRepository.cs 407
public static IDisposable Read(this ReaderWriterLockSlim obj)
{
return new WriteLockToken(obj);
}
public static IDisposable Write(this ReaderWriterLockSlim obj)
{
return new WriteLockToken(obj);
}
private sealed class WriteLockToken : IDisposable
{
private ReaderWriterLockSlim _sync;
public WriteLockToken(ReaderWriterLockSlim sync)
{
_sync = sync;
sync.EnterWriteLock();
}
public void Dispose()
{
if (_sync != null)
{
_sync.ExitWriteLock();
_sync = null;
}
}
}The functions Read and Write have the same bodies, and that's what the analyzer is telling us. The EnterWriteLock method is used to enter the lock in write mode. If you want to enter the lock in read mode, use the EnterReadLock method, which allows a resource to be read by several threads at a time.
The developers should check this code because it's very likely to contain a bug – all the more so since there's an unused class found in the code:
private sealed class ReadLockToken : IDisposable
{
private ReaderWriterLockSlim _sync;
public ReadLockToken(ReaderWriterLockSlim sync)
{
_sync = sync;
sync.EnterReadLock();
}
public void Dispose()
{
if (_sync != null)
{
_sync.ExitReadLock();
_sync = null;
}
}
}PVS-Studio diagnostic message: V3021 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 SkiaEncoder.cs 537
public string EncodeImage(string inputPath,
DateTime dateModified,
string outputPath,
bool autoOrient,
ImageOrientation? orientation,
int quality,
ImageProcessingOptions options,
ImageFormat selectedOutputFormat)
{
if (string.IsNullOrWhiteSpace(inputPath))
{
throw new ArgumentNullException("inputPath");
}
if (string.IsNullOrWhiteSpace(inputPath))
{
throw new ArgumentNullException("outputPath");
}
....
}The developer must have cloned the first four lines but forgot to change the name of the variable being checked from inputPath to outputPath. There are several lines further on where outputPath is used without a prior null check, which means an exception may be thrown.
PVS-Studio diagnostic messages:
private bool processWebSocketFrame(WebSocketFrame frame)
{
return frame.IsCompressed && _compression == CompressionMethod.None
? processUnsupportedFrame(....)
: frame.IsFragmented
? processFragmentedFrame(frame)
: frame.IsData
? processDataFrame(frame)
: frame.IsPing
? processPingFrame(frame)
: frame.IsPong
? processPongFrame(frame)
: frame.IsClose
? processCloseFrame(frame)
: processUnsupportedFrame(....);
}
private bool processUnsupportedFrame(....)
{
processException(....);
return false;
}
private bool processDataFrame(WebSocketFrame frame)
{
....
return true;
}
private bool processPingFrame(WebSocketFrame frame)
{
var mask = Mask.Unmask;
return true;
}
private bool processPongFrame(WebSocketFrame frame)
{
_receivePong.Set();
return true;
}
private bool processCloseFrame(WebSocketFrame frame)
{
var payload = frame.PayloadData;
close(payload, !payload.ContainsReservedCloseStatusCode, false);
return false;
}I have checked fewer projects than my PVS-Studio teammates have so far, and this probably explains why I have never before seen a code snippet of 13 lines that would trigger 7 warnings at once (i.e. slightly more than one warning per two lines). That's why I'm including this case into the article. Below is a step-by-step analysis of the problem fragment.
I hope it wasn't too tedious to follow. The remaining examples aren't that complicated.
PVS-Studio diagnostic message: V3085 The name of 'RtpHeaderBytes' field in a nested type is ambiguous. The outer type contains static field with identical name. HdHomerunUdpStream.cs 200
public class HdHomerunUdpStream : LiveStream, IDirectStreamProvider
{
....
private static int RtpHeaderBytes = 12;
public class UdpClientStream : Stream
{
private static int RtpHeaderBytes = 12;
private static int PacketSize = 1316;
private readonly MediaBrowser.Model.Net.ISocket _udpClient;
bool disposed;
....
}
....
}The nested class UdpClientStream has a field whose name is identical to that of a field of the enclosing class HdHomerunUdpStream. It's not a bug but it's a good reason to check this code again to make sure it's correct. Having variables with identical names makes it easy to accidentally use one of them instead of the other, resulting in the program's unexpected behavior, while the compiler won't say a word.
PVS-Studio diagnostic messages:
public class Lmhosts
{
public static NbtAddress GetByName(string host)
{
lock (typeof(Lmhosts))
{
return GetByName(new Name(host, 0x20, null));
}
}
internal static NbtAddress GetByName(Name name)
{
lock (typeof(Lmhosts))
{
....
}
}
}The analyzer is warning about an unsafe lock here. Using lock in a way like that isn't recommended because the lock object is publicly accessible and can be locked on in some other place, and the developer who first used this object may never know about that. This may lead to a deadlock.
Ideally, you should use a private field for locking, for example:
private Object locker = new Object();
public static NbtAddress GetByName(string host)
{
lock (locker)
{
return GetByName(new Name(host, 0x20, null));
}
}PVS-Studio diagnostic message: V3142 Unreacheble code detected. It is possible that an error is present. HdHomerunHost.cs 621
protected override async Task<ILiveStream> GetChannelStream(....)
{
....
var enableHttpStream = true;
if (enableHttpStream)
{
mediaSource.Protocol = MediaProtocol.Http;
var httpUrl = channelInfo.Path;
// If raw was used, the tuner doesn't support params
if (!string.IsNullOrWhiteSpace(profile) &&
!string.Equals(profile, "native",
StringComparison.OrdinalIgnoreCase))
{
httpUrl += "?transcode=" + profile;
}
mediaSource.Path = httpUrl;
return new SharedHttpStream(....);
}
return new HdHomerunUdpStream(....);
}The analyzer says that the last line in this snippet will never execute. And what is the purpose of declaring the variable enableHttpStream, assigning true to it, and checking it right afterwards?
Perhaps this code is simply redundant, but it needs revising anyway.
PVS-Studio diagnostic message: V3083 Unsafe invocation of event 'RefreshStarted', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ProviderManager.cs 943
public void OnRefreshStart(BaseItem item)
{
....
if (RefreshStarted != null)
{
RefreshStarted(this, new GenericEventArgs<BaseItem>(item));
}
}The analyzer warns us about a potentially unsafe call of the RefreshStarted event handler.
Let's figure out why this call is unsafe. Suppose the event gets unsubscribed from in another thread at the moment between checking the event for null and calling the event handler in the body of the if statement. If there are no subscribers left, the RefreshStarted event will become null, but in the thread where the null check has already passed, the call will be executed anyway:
RefreshStarted(this, new GenericEventArgs<BaseItem>(item));This will result in throwing a NullReferenceException.
PVS-Studio diagnostic message: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 142, 152. LocalImageProvider.cs 142
private void PopulateImages(....)
{
....
// Logo
if (!isEpisode && !isSong && !isPerson)
{
added = AddImage(....);
if (!added)
{
added = AddImage(....);
}
}
// Art
if (!isEpisode && !isSong && !isPerson)
{
AddImage(....);
}
....
}The two if statements have identical conditions, but their bodies are different. I'm not sure if this is a bug or just redundant code. Perhaps it's OK and the developer simply wanted to explicitly differentiate between two actions, one of which has to do with "Logo" and the other with "Art", whatever those are.
PVS-Studio diagnostic message: V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. LiveTvManager.cs 1085
private async Task RefreshChannelsInternal(....)
{
....
double progressPerService = _services.Length == 0
? 0
: 1 / _services.Length;
....
}This code contains an integer division, with the resulting value being cast to a floating-point type, which doesn't look a right thing to do.
Actually, the progressPerService variable will have the value 1.0 only if _services.Length = 1. With any other value of _services.Length, the result will be 0.0.
I think what should be written instead is the following:
double progressPerService = _services.Length == 0
? 0
: (double)1 / _services.Length;PVS-Studio diagnostic message: V3050 Possibly an incorrect HTML. The </u> closing tag was encountered, while the </i> tag was expected. SrtParserTests.cs 64
public void TestParse()
{
var expectedSubs =
new SubtitleTrackInfo
{
TrackEvents = new SubtitleTrackEvent[]
{
....
new SubtitleTrackEvent
{
Id = "6",
StartPositionTicks = 330000000,
EndPositionTicks = 339990000,
Text =
"This contains nested <b>bold,
<i>italic, <u>underline</u> and
<s>strike-through</s></u></i></b> HTML tags"
},
....
}
};
}Note this line "<u>underline</u> ". It already has a closing tag </u>. Then we see the following text: </s></u></i></b> HTML tags"
There's an extra closing tag </u> here, which is what the analyzer points out.
PVS-Studio diagnostic message: V3051 An excessive type check. The object is already of the 'Exception' type. SmbFileInputStream.cs 107
protected internal virtual IOException SeToIoe(SmbException se)
{
IOException ioe = se;
Exception root = se.GetRootCause();
if (root is TransportException)
{
ioe = (TransportException)root;
root = ((TransportException)ioe).GetRootCause();
}
if (root is Exception)
{
ioe = new IOException(root.Message);
ioe.InitCause(root);
}
return ioe;
}Frankly, I don't quite understand what the developer meant by this code. The analyzer says the second if statement's condition checks if the root object is compatible with its own type. This is probably just redundant code, but it does look strange and I recommend revising it.
The developers of Emby have done a great job in every way (the project is 215,539 LOC long, 4.6% of which are comments). They did well, I mean it. But PVS-Studio deserves praise too: it produced 113 High-level, 213 Medium-level, and 112 Low-level warnings. Some of them were false positives, but most of the bugs weren't mentioned here because they were very much the same. For instance, the V3022 diagnostic (always false/true condition) alone was triggered 106 times. Of course, I could have filtered off the false positives and included the rest into the article, but it would have become too boring to read.
I hope I managed to show how static analysis helps in software development. Obviously, one-time checks aren't enough; you should use static analysis on a regular basis. This topic is discussed in more detail in the article "Godot: on regular use of static analyzers".