Emby source code analysis with PVS-Studio analyzer
Emby is a fairly well-known media server, along with Plex and Kodi. In this article, we will try to verify its source code using the PVS-Studio static analyzer. On the official Emby website there is a small mark “Built with ReSharper”. Well, it’s even more interesting.
PVS-Studio code analyzer
PVS-Studio runs on 64-bit systems on Windows, Linux and macOS. He knows how to look for errors in the source code of programs written in C, C ++, C # and Java.
Audited Project
Emby is a media server whose source code can be found on Github. It allows you to organize broadcasts and makes it possible to access media content (video, audio, photos) from any device. Squeeze from the official site Emby:
- Emby automatically converts your media on the fly for playback on any device;
- Extensive parental controls make it easy to control access to content, which will be relevant for families with small children;
- Emby organizes your media into simple and elegant presentations. They will never look the same;
- Streaming data with synchronization in the cloud;
- Easily share your media files with friends and family;
- And much more.
Pieces of code that caught attention when examining an analyzer report
PVS-Studio warning: 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 != "https://habr.com/" && c != '[' &&
c != ']' && c != '?' && c != '=' && c != '{' && c != '}'))
continue;
....
}The analyzer detected a duplicate subexpression c! = '<'. Perhaps an error crept into this code, and in place '<' there must have been something else. Or, more likely, the second subexpression is superfluous and you just need to delete it.
PVS-Studio warning: 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;
}The analyzer again found a typo associated with repeated subexpressions. Looking ahead, I’ll say that in this project there are a bit too many similar errors made by carelessness. I do not blame the creators of Emby at all, we all are sometimes scattered in this way (examples, examples, examples), and in order to be safe from such errors, there is just a static analysis.
PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. SqliteItemRepository.cs 5648
private QueryResult> 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 is very similar to the problem that appeared due to copy-paste, because block bodies if and else completely identical. Why did you need to check the size of the array typesToCount, if, regardless of the number of elements recorded in it, the same code will be executed, it is known only to developers.
PVS-Studio warning: V3005 The '_validProviderIds' variable is assigned to itself. BaseNfoParser.cs 77
private Dictionary _validProviderIds;
....
public void Fetch(....)
{
....
_validProviderIds = _validProviderIds = new Dictionary<....>(....);
....
} Again, a typo, which entailed the assignment of a variable to itself. It is worth rechecking this piece of code.
PVS-Studio warning: 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();
Chapters = new List();
Chapters = new List();
TitleNumber = titleNum;
} Inattention and typos, again ... Variable Chapters value is assigned twice. It is clear that this extra code does not threaten anything bad, but nonetheless. It makes no sense to linger on this example, let's move on.
PVS-Studio warning: 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;
}
}
}Function bodies Read and Write are absolutely identical to each other, as the analyzer warning text indicates. Method EnterWriteLock needed in order to enter the lock in recording mode. For reading mode, use the method EnterReadLock, allowing the ability to simultaneously read the resource in multiple threads.
The developer should take a closer look at this section of the code, perhaps there is a mistake. Moreover, the following class was encountered in the code, which is not used:
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 warning: 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");
}
....
}Here, apparently, the developer copied the first four lines, but forgot to change the variable being checked from inputPath in outputPath. There are several lines in the code that use outputPath without checking for null, which can lead to an exception being thrown.
PVS-Studio warnings:
- V3022 Expression 'processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null)' is always false. WebSocket.cs 462
- V3022 Expression 'processCloseFrame (frame)' is always false. WebSocket.cs 461
- V3022 Expression 'frame.IsClose? processCloseFrame (frame): processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null) 'is always false. WebSocket.cs 460
- V3022 Expression 'processPongFrame (frame)' is always true. WebSocket.cs 459
- V3022 Expression 'processPingFrame (frame)' is always true. WebSocket.cs 457
- V3022 Expression 'processDataFrame (frame)' is always true. WebSocket.cs 455
- V3022 Expression is always false. WebSocket.cs 448
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;
}In my account, there are not so many, in comparison with the rest of the PVS-Studio team, verified by the project analyzer, and maybe this is somehow connected with the fact that I first see that as many as 13 analyzer warnings are issued on 13 lines of code (a little more than every second line). Therefore, I believe that this code deserves to be included in the article. Next, I will write down what the analyzer swears specifically at.
- First, the expression is considered frame.IsCompressed && _compression == CompressionMethod.None and if the expression is true, the method will be executed processUnsupportedFramewhich in any case returns false (hence the first warning of the analyzer). If it was false, then move on to the next point.
- Next, the value is checked. frame.IsFragmented. There are no problems, so let's move on.
- Here the value is checked frame.IsData. If true, then the method processDataFrame in any case, will return true. Hence the second analyzer warning.
- Then we check frame.IsPing. Truly - processPingFrame will return true. This is the third analyzer warning.
- Look at frame.IsPong. Everything is similar to the previous one.
- And the last one: frame.IsClose. processCloseFrame and processUnsupportedFrame in any case return false.
I hope I did not bore you. It will be simpler further.
PVS-Studio warning: 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;
....
}
....
}In a nested class UdpClientStream uses a field with a name similar to that used in the outer class HdHomerunUdpStream. This is not a mistake, but it is a good reason to check if everything is okay with this code. Its problem is that there is a chance to confuse these two variables, and the code will not work as intended by the developer, although it will not cause complaints from the compiler.
PVS-Studio warnings:
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. Lmhosts.cs 49
- V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. Lmhosts.cs 57
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 warns that an insecure lock is used in this code fragment. So use lock This is undesirable because the object used for locking is shared and can be used for locking in another place without the knowledge of the developer who used this object for the first time. The result will be a deadlock on the same object.
Ideally, you should use a private field for locking, for example, like this:
private Object locker = new Object();
public static NbtAddress GetByName(string host)
{
lock (locker)
{
return GetByName(new Name(host, 0x20, null));
}
}PVS-Studio warning: V3142 Unreacheble code detected. It is possible that an error is present. HdHomerunHost.cs 621
protected override async Task 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 indicates that the last line in this code fragment will never be executed. And why does the developer declare and set the enableHttpStream variable to true and then immediately check this variable in the if statement?
Perhaps this code is simply redundant. In any case, it is worth double-checking.
PVS-Studio warning: 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(item));
}
} This method uses a potentially unsafe event handler call. RefreshStarted, as indicated by the analyzer.
Now I will explain why this call RefreshStarted in the code, the analyzer considered it unsafe. Let's say, at the time between checking for null and directly calling the event handler in the body if An unsubscribe from an event in another thread has been produced. And if there are no more subscribers RefreshStarted will matter null. But in another thread, where is the check for null already occurred, the following line will be executed:
RefreshStarted(this, new GenericEventArgs(item)); This will throw an exception. NullReferenceException.
PVS-Studio warning: V3029 The conditional expressions of the 'if' statements located 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(....);
}
....
}In this piece of code, next to each other are two operators if with the same conditions. Moreover, the bodies of the operators if vary. Difficult to say whether this is a mistake or redundant code. Perhaps everything is fine and the author just wanted to logically separate two different actions, one of which is associated with a certain “Logo”, and the second with a certain “Art”.
PVS-Studio warning: 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 the operation of dividing integer data types. The resulting value is implicitly converted to a floating-point type, which is extremely suspicious.
Actually, the variable progressPerService will be equal to 1.0 only if _services.Length = 1. For any other values _services.Length the result will be 0.0.
Apparently, it should be written here:
double progressPerService = _services.Length == 0
? 0
: (double)1 / _services.Length;PVS-Studio warning: V3050 Possibly an incorrect HTML. The closing tag was encountered, while the 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 bold,
italic, underline and
strike-through HTML tags"
},
....
}
};
}Pay attention to the line
underlineIt already has an end tag. . Next we see this text:
HTML tags"Here is the closing tag unnecessary, as indicated by the analyzer.
PVS-Studio warning: 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;
}And here, frankly, it is not entirely clear what the developer generally wanted to achieve. The analyzer warns that in the second statement if object check in progress root for compatibility with your own type. Most likely, this is just redundant code, but it looks suspicious, the developer should take a closer look at it.
Conclusion
In every sense, the developers did a great job creating Emby (the project has 215 539 lines of code, 4.6% - comments). They are great, really. However, the analyzer also tried by issuing warnings level High - 113, Medium - 213 and Low - 112. Some of them were false positives, but most of the errors did not appear in the article due to the fact that they were similar to each other.
For example, warnings alone V3022 (expression is always false / true) 106 pieces were accumulated. You can, of course, check which part of them was false positives, and add the rest to the article, but this will turn out a very boring text that no one will read.
I hope I managed to demonstrate the usefulness of the static analyzer in development. Of course, one-time checks are not enough, and a static analyzer must be used on an ongoing basis. You can read more about this in the article "Godot: on the issue of regular use of static code analyzers"
If you want to share this article with an English-speaking audience, then please use the link to the translation: Ekaterina Nikiforova. Checking Emby with PVS-Studio.
