Checking Orleans with the PVS-Studio analyzer
Orleans is a cross-platform framework for creating scalable cloud applications. This software is developed by Microsoft, and PVS-Studio often checks its projects. Let's see how many suspicious places our analyzer can find this time.
Introduction
Orleans scales from an on-premises server to cloud-based distributed applications. The project's main feature is a programming model that simplifies the development of concurrent distributed systems.
The project code is almost entirely written in C#. You can find it in the?repository on GitHub. We checked the code with the PVS-Studio analyzer. As mentioned above, the Orleans project was developed by Microsoft, which makes it interesting for analysis. We have quite a?lot of articles?about checking Microsoft open-source projects, I encourage you to read them.
As a result of analysis, we got 229 warnings — 38 with High level of certainty, 106 with Medium level, and 85 with Low level. In this article, I'll describe the most interesting ones.
Non-obvious initialization
Issue 1
public abstract class SystemTarget : ....
{
....
internal SystemTarget(SystemTargetGrainId grainId,
SiloAddress silo,
bool lowPriority,
ILoggerFactory loggerFactory)
{
this.id = grainId;
this.Silo = silo;
this.ActivationAddress = GrainAddress.GetAddress(this.Silo,
this.id.GrainId,
this.ActivationId); // <=
this.IsLowPriority = lowPriority;
this.ActivationId = ActivationId // <=
.GetDeterministic(grainId.GrainId);
this.timerLogger = loggerFactory.CreateLogger<GrainTimer>();
this.logger = loggerFactory.CreateLogger(this.GetType());
}
....
}
PVS-Studio's warning:?V3128?The 'ActivationId' property is used before it is initialized in constructor. SystemTarget.cs 83
The analyzer detects that one of the properties in the constructor is used before initialization. The?this.ActivationAddress?property is assigned the value which was obtained as a result of the?GrainAddress.GetAddress?method's execution.?this.ActivationId?is passed as one of the parameters to this method. Well, it looks like a correct operation. Except one thing. The?this.ActivationId?property is initialized after it's used. Perhaps the developer confused the initialization order of the properties mentioned above.
The same then and else
Issue 2
public virtual async Task ConfirmOneAndCancelOne(bool useTwoSteps = false,
bool reverseOrder = false)
{
....
if (useTwoSteps)
{
if (reverseOrder) // <=
{
etag = await stateStorage.Store(etag, metadata,
emptyPendingStates, 1, null);
_ = await stateStorage.Store(etag, metadata,
emptyPendingStates, null, 1);
}
else
{
etag = await stateStorage.Store(etag, metadata,
emptyPendingStates, 1, null);
_ = await stateStorage.Store(etag, metadata,
emptyPendingStates, null, 1);
}
}
else
{
_ = await stateStorage.Store(etag, metadata,
emptyPendingStates, 1, 1);
}
....
}
PVS-Studio's warning:?V3004?The 'then' statement is equivalent to the 'else' statement. TransactionalStateStorageTestRunner.cs 327
The analyzer warns that the then and else branches of the conditional?if?operator are the same. Indeed, it is very strange — the same actions are performed regardless of the value of the?reverseOrder?argument. Most likely, the code is not completed. Or it's just a typo.
If the developer intended to make these two actions the same, then I think this fragment needs an explanatory comment.
Ambiguous for
Issue 3
private class BatchOperation
{
private readonly List<TableTransactionAction> batchOperation;
....
public async Task Flush()
{
if (batchOperation.Count > 0)
{
try
{
....
batchOperation.Clear(); // <=
keyIndex = -1;
if (logger.IsEnabled(LogLevel.Trace))
{
for (int i = 0; i < batchOperation.Count; i++) // <=
{
logger.LogTrace(....)
}
}
}
catch (Exception ex)
{
....
}
}
}
}
PVS-Studio's warning:?V3116?Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. AzureTableTransactionalStateStorage.cs 345
Look at the?for?loop. It should help output some debugging information, but it won't — the?batchOperation?collection is cleared before this loop. It's better to delete elements from the list after the loop.
Issue 4
public static MethodInfo GetMethodInfoOrDefault(....)
{
foreach (var method in interfaceType.GetMethods( BindingFlags.Public
| BindingFlags.NonPublic
| BindingFlags.Instance))
{
....
var parameters = method.GetParameters();
if (parameters.Length != parameterTypes.Length)
{
continue;
}
for (int i = 0; i < parameters.Length; i++)
{
if (!parameters[0].ParameterType.Equals(parameterTypes[i])) // <=
{
continue;
}
}
return method;
}
....
}
PVS-Studio's warning:?V3102?Suspicious access to element of 'parameters' object by a constant index inside a loop. OrleansGeneratedCodeHelper.cs 267
The analyzer was triggered by a loop in which an array element is accessed via the constant index. Look at the?if (parameters.Length != parameterTypes.Length)?condition. If it is true, the?continue?statement is triggered. Therefore, the collections should be of the same size to execute the subsequent code. This was most likely made to further compare pairs of corresponding elements of these collections. However, in the?for?body, the first element is always taken from the?parameters?collection.
We must say that there's another ambiguous point. Using?for?is pointless since no actions are performed there except for skipping to a new iteration of this loop. Perhaps the developer expected to move to the next iteration of the external loop, but something went wrong.
This situation can be fixed by adding a flag to move to a new iteration of?foreach?and changing the index for?parameters?to?i. The code will look like this:
public static MethodInfo GetMethodInfoOrDefault(....)
{
foreach (var method in interfaceType.GetMethods( BindingFlags.Public
| BindingFlags.NonPublic
| BindingFlags.Instance))
{
....
bool flag = false;
for (int i = 0; i < parameters.Length; i++)
{
if (!parameters[i].ParameterType.Equals(parameterTypes[i]))
{
flag = true;
break;
}
}
if(flag)
continue;
return method;
}
....
}
Issues with while
Issue 5
public async ValueTask<ConnectionContext> AcceptAsync(....)
{
if (await _acceptQueue.Reader.WaitToReadAsync(....))
{
while (_acceptQueue.Reader.TryRead(out var item))
{
var remoteConnectionContext = item.Connection;
var localConnectionContext = ....
item.ConnectionAcceptedTcs.TrySetResult(true);
return localConnectionContext; // <=
}
}
return null;
}
PVS-Studio's warning:?V3020?An unconditional 'return' within a loop. InMemoryTransportListenerFactory.cs 117
Now look at the?while loop.?The loop's body uses the?return?operator which will be executed on the first iteration. Perhaps the developer meant that the code inside the loop should work only once. If so, why not use?if? This will make the code more understandable. It's also possible that this loop is necessary here. In this case, the?return?operator must be executed depending on some condition.
Issue 6
public static TService UnwrapService<TService>(object caller, TService service)
{
while ( service is IServiceHolder<TService>
&& caller is TService callerService)
{
return callerService;
}
....
}
PVS-Studio's warning:?V3020?An unconditional 'return' within a loop. OrleansGeneratedCodeHelper.cs 99
This issue is similar to the previous one. The?return?operator is used in the?while?body. As already mentioned in this article, using?while?like this is pointless — the loop will have only one iteration. Perhaps there should be some condition for using the?return?operator.
Potential dereference of the null reference
Issue 7
private int CheckLocalHealthCheckParticipants(DateTime now,
List<string> complaints)
{
var score = 0;
foreach (var participant in _healthCheckParticipants)
{
try
{
if (!participant.CheckHealth(_lastHealthCheckTime, out var reason)) // <=
{
_log.LogWarning(...., participant?.GetType().ToString(), reason); // <=
complaints?.Add($".... {participant?.GetType().ToString()} ...."); // <=
++score;
}
}
catch (Exception exception)
{
_log.LogError(exception, ...., participant?.GetType().ToString()); // <=
Complaints?.Add($".... {participant?.GetType().ToString()} ...."); // <=
++score;
}
}
_lastHealthCheckTime = now;
return score;
}
PVS-Studio's warning:?V3095?The 'participant' object was used before it was verified against null. Check lines: 282, 284. LocalSiloHealthMonitor.cs 282
The analyzer detected that the?participant?variable was used before it was checked for?null. It's strange that this variable is accessed without any check:
领英推荐
if (!participant.CheckHealth(_lastHealthCheckTime, out var reason))
All subsequent accesses to the same variable (4 accesses, actually) are checked. Apparently, the developer expected that?participant?can be?null. Note that?CheckHealth?is not an extension method. If we call such method from a?null?variable, then?NullReferenceException?will be thrown.
Although a potentially dangerous code fragment is in the?try?block, it's unlikely that the developer wanted to catch exceptions of this type. This conclusion can be made based on the number of?null?checks in this block.
Issue 8
public Silo(ILocalSiloDetails siloDetails, IServiceProvider services)
{
....
foreach (ILifecycleParticipant<ISiloLifecycle> participant
in namedLifecycleParticipantCollection?.GetServices(this.Services)
?.Select(....))
{
participant?.Participate(this.siloLifecycle);
}
....
}
PVS-Studio's warning:?V3153?Enumerating the result of null-conditional access operator can lead to NullReferenceException. Silo.cs 180
Look at the collection for which iteration will be performed in?foreach. This collection is a result of calling the?GetServices?and?Select?methods. The calls are made using the '?.' operator. Most likely, the developer expected that?null?could be obtained as a result of accessing?namedLifecycleParticipantCollection?or when calling the?GetServices?method.
In this case,?namedLifecycleParticipantCollection?.GetServices(....)?.Select(....)?will also be?null. An attempt to iterate the collection with?null?in?foreach?will lead to?NullReferenceException. Unfortunately, the null conditional operator here is useless. If you want a detailed explanation of this problem, you can read this?article.
To avoid such a situation, use the '??' operator. In this case, if '?.' returns?null, the exception won't be thrown.
The correct version of the loop looks like this:
foreach (ILifecycleParticipant<ISiloLifecycle> participant
in namedLifecycleParticipantCollection?.GetServices(this.Services)
?.Select(....)
?? Enumerable.Empty<ILifecycleParticipant<ISiloLifecycle>>)
Issue 9
public void FailMessage(Message msg, string reason)
{
if (msg != null && msg.IsPing()) // <=
{
this.Log.LogWarning("Failed ping message {Message}", msg);
}
MessagingStatisticsGroup.OnFailedSentMessage(msg);
if (msg.Direction == Message.Directions.Request) // <=
{
if (this.Log.IsEnabled(LogLevel.Debug)) ....;
this.messageCenter.SendRejection(....);
}
else
{
this.MessagingTrace.OnSiloDropSendingMessage(....);
}
}
PVS-Studio's warning:?V3125?The 'msg' object was used after it was verified against null. Check lines: 275, 269. SiloConnection.cs 275
Potential dereference of the null reference. Again. In this example, before the?msg?variable is accessed for the first time, the variable is checked for?null. After that, the variable is passed as an argument to the?MessagingStatisticsGroup.OnFailedSentMessage?method, where it's checked again.
internal static void OnFailedSentMessage(Message msg)
{
if (msg == null || !msg.HasDirection) return;
....
}
However, there's no check in the second?if?statement of the?FailMessage?method.?As mentioned above, dereferencing of the null reference will lead to?NullReferenceException.
We often see such errors when we check open-source projects. You can see examples?here.
Issue 10
private async Task ReadTableAndStartTimers(IRingRange range,
int rangeSerialNumberCopy)
{
....
try
{
....
ReminderTableData table = await reminderTable.ReadRows(....);
....
if (null == table && reminderTable is MockReminderTable) return; // <=
var remindersNotInTable = ....
if (logger.IsEnabled(LogLevel.Debug))
logger.Debug(...., table.Reminders.Count, ....); // <=
....
}
catch (Exception exc)
{
....
}
}
PVS-Studio's warning:?V3125?The 'table' object was used after it was verified against null. Check lines: 306, 303. LocalReminderService.cs 306
This warning is similar to the previous one.?Here the?table?variable is checked for?null?and after that it is accessed without any check. As in the previous example, if?table?is?null, accessing its property will result in an exception thrown.
Suspicious shifts
Issue 11, 12
public static void WriteField<TBufferWriter>
(ref Writer<TBufferWriter> writer,
uint fieldIdDelta,
Type expectedType,
long value) where TBufferWriter : IBufferWriter<byte>
{
ReferenceCodec.MarkValueField(writer.Session);
if (value <= int.MaxValue && value >= int.MinValue) // <=
{
if (value > 1 << 20 || -value > 1 << 20)
{
writer.WriteFieldHeader(fieldIdDelta,
expectedType,
CodecFieldType,
WireType.Fixed32);
writer.WriteInt32((int)value);
}
else
{
writer.WriteFieldHeader(fieldIdDelta,
expectedType,
CodecFieldType,
WireType.VarInt);
writer.WriteVarInt64(value);
}
}
else if (value > 1 << 41 || -value > 1 << 41) // <=
{
writer.WriteFieldHeader(fieldIdDelta,
expectedType,
CodecFieldType,
WireType.Fixed64);
writer.WriteInt64(value);
}
else
{
writer.WriteFieldHeader(fieldIdDelta,
expectedType,
CodecFieldType,
WireType.VarInt);
writer.WriteVarInt64(value);
}
}
Here PVS-Studio issues two warnings at once:
Let's inspect the first warning. In the?if (value > 1 << 41 || -value > 1 << 41)?condition, 1 is shifted bitwise. After that the result is compared with the?value?variable.?The problem is that 1 has the?Int32?type, which size is 32 bits. So, a shift of 41 bits is equivalent to the shift of 9. A shift of more bits than the size of the left operand of the '>>' operator looks strange.
In the condition, a comparison is made with the?value?variable. It has the?long?type, which is an alias of the?Int64 type.?Also, in the then block of this condition, the?WriteInt64?method is called. This method takes a variable of the?Int64?type as an argument. The points mentioned above make us doubt whether the implementation of the shift was correct.
To understand the second warning, we need to inspect one more condition —?if (value <= int.MaxValue && value>= int.MinValue). In the else block of this condition,?value?will not be in the?Int32?type range. Hence the?if (value > 1 << 41 || -value > 1 << 41)?condition will always be true.
Most likely, the developer believed that 1, in regards to which the shift is made in the?if (value > 1 << 41 || -value > 1 << 41)?condition, is of the?Int64?type, but it is not.
For correct implementation, the?L?suffix should be used. After making this fix, the condition will look like this:
if (value > 1L << 41 || -value > 1L << 41)
Incorrect message
Issue 13
public Exception DeserializeException<TInput>(....)
{
if (!_typeConverter.TryParse(typeName, out var type))
{
....
}
else if (typeof(Exception).IsAssignableFrom(type))
{
....
}
else
{
throw new NotSupportedException("Type {type} is not supported");
}
}
PVS-Studio's warning:?V3138?String literal contains potential interpolated expression. Consider inspecting: type. ExceptionCodec.cs 367
The analyzer detected a string that most likely contains an interpolated expression, but the '$' symbol was not used. Look at the last else block. It creates an object of the?NotSupportedException?type. A string is passed to the constructor of this object. I doubt that the developer wanted to send messages like "Type {type} is not supported". Most likely, the value of the?type?variable should be substituted instead of the "{type}" substring. The code will look like this:
throw new NotSupportedException($"Type {type} is not supported");
Conclusion
Summing up, we can say that the warnings were quite diverse. The article presents both errors and minor mistakes in the code. Anyway, it's better to fix them all.
A third of the warnings described in this article is about the potential dereference of the null reference. This is not surprising – such warnings were issued the most. Perhaps the developers of Orleans should investigate this case.
You can check your code with the analyzer too. Just download it?here. You can try it for free, some help with the code never hurts :).
Thank you and see you soon!