Our website uses cookies to enhance your browsing experience.
Webinar: Let's make a programming language. Lexer - 29.04
Semantic Kernel is a Microsoft's SDK for integrating AI models into applications. Can PVS-Studio static analyzer find defects in the source code of a project like this? This article answers this question. Enjoy reading!
Projects that involve AI integration increasingly shape everyday development. One such project is Semantic Kernel. It's an SDK for building AI agents and orchestrating LLM scenarios, and Microsoft actively develops it.
But underneath even the most modern solutions, you'll find ordinary C# code—with all the usual problems that come with it. We'll try to confirm that today.
To analyze the C# part of the project, we used PVS-Studio static analyzer version 7.41. The source code we checked corresponds to this commit.
Note that we'll only discuss the most interesting code snippets to keep the article concise.
Let's start breaking down suspicious spots!
Code snippet 1
internal static async Task
CreateToolsAsync(this AgentDefinition agentDefinition,
BedrockAgent agent,
CancellationToken cancellationToken)
{
....
if (knowledgeBase.Options
?.TryGetValue(KnowledgeBaseId,
out var value) ?? false && value is not null
&& value is string)
{
var knowledgeBaseId = value as string;
var description = knowledgeBase.Description ?? string.Empty;
await agent.AssociateAgentKnowledgeBaseAsync(knowledgeBaseId!,
description,
cancellationToken)
.ConfigureAwait(false)
}
....
}The PVS-Studio warning: V3177 The 'false' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. BedrockAgentDefinitionExtensions.cs 44
The developer likely intended this condition to work as follows: if knowledgeBase.Options?.TryGetValue(KnowledgeBaseId, out var value) is not null then check that value is not null && value is string. But the actual behavior differs. The && operator takes higher priority than ??, so the checks on value never execute.
Another clue that an error lurks here: the right operand of ??. It looks like false && value is not null && value is string. The outcome of that expression is obvious. The condition contains only && operators, and one operand is false. So, the entire expression is always false.
To fix it, the author needs to add parentheses:
(knowledgeBase.Options
?.TryGetValue(KnowledgeBaseId,
out var value) ?? false) && value is not null
&& value is stringNote that 4 other places in the source code contain the same issue:
Code snippet 2
private static readonly RestApiParameterFilter s_restApiParameterFilter =
(RestApiParameterFilterContext context) =>
{
if ( ("me_sendMail".Equals(context.Operation.Id,
StringComparison.OrdinalIgnoreCase)
|| ("me_calendar_CreateEvents".Equals(context.Operation.Id,
StringComparison.OrdinalIgnoreCase))
&& "payload".Equals(context.Parameter.Name,
StringComparison.OrdinalIgnoreCase)))
{
context.Parameter
.Schema = TrimPropertiesFromRequestBody(context.Parameter.Schema);
return context.Parameter;
}
return context.Parameter;
};The PVS-Studio warning: V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. CopilotAgentBasedPlugins.cs 212
The outermost parentheses wrap the entire if condition, making them pointless. The developer probably intended this logic: apply the payload parameter name check to both operations (me_sendMail and me_calendar_CreateEvents). But in reality, the payload check applies only to the second operation because && has a higher priority than ||. To fix it, the developer needs to parenthesize only the checks for me_sendMail and me_calendar_CreateEvents, not the entire condition.
Code snippet 3
private static string? GetAudioOutputMimeType(ChatAudioOptions? audioOptions)
{
if (audioOptions is null)
{
return null;
}
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav) // <=
{
return "audio/wav";
}
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Mp3)
{
return "audio/mp3";
}
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Opus)
{
return "audio/opus";
}
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav) // <=
{
return "audio/wav";
}
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Flac)
{
return "audio/flac";
}
if (audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Pcm16)
{
return "audio/pcm16";
}
throw new NotSupportedException
(
$"Unsupported audio output format '{audioOptions.OutputAudioFormat}'. " +
"Supported formats are 'wav', 'mp3', 'opus', 'flac' and 'pcm16'."
);
}The 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 ClientCore.ChatCompletion.cs 985
The GetAudioOutputMimeType method contains the same check twice: audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav. The repeated check for ChatOutputAudioFormat.Wav is unreachable code—the method returns when the first match occurs. This is a typical copy-paste error.
Interestingly, the ChatOutputAudioFormat struct declares a ChatOutputAudioFormat Aac format. This is the only format the GetAudioOutputMimeType method doesn't handle. Perhaps it should replace one of the audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav check.
public readonly partial struct ChatOutputAudioFormat
: IEquatable<ChatOutputAudioFormat>
{
....
public static ChatOutputAudioFormat Wav { get; } =
new ChatOutputAudioFormat(WavValue);
public static ChatOutputAudioFormat Aac { get; } = // <=
new ChatOutputAudioFormat(AacValue);
public static ChatOutputAudioFormat Mp3 { get; } =
new ChatOutputAudioFormat(Mp3Value);
public static ChatOutputAudioFormat Flac { get; } =
new ChatOutputAudioFormat(FlacValue);
public static ChatOutputAudioFormat Opus { get; } =
new ChatOutputAudioFormat(OpusValue);
public static ChatOutputAudioFormat Pcm16 { get; } =
new ChatOutputAudioFormat(Pcm16Value);
....
}Code snippet 4
public async Task<KernelSearchResults<TextSearchResult>>
GetTextSearchResultsAsync(string query,
TextSearchOptions? searchOptions = null,
CancellationToken cancellationToken = default)
{
var searchResult = await this.SearchInternalAsync(query,
searchOptions,
cancellationToken)
.ConfigureAwait(false);
var results =
searchResult.Select(x =>
new TextSearchResult(x.Text ?? string.Empty)
{ Name = x.SourceName, Link = x.SourceLink });
return
new(searchResult.Select(x =>
new TextSearchResult(x.Text ?? string.Empty)
{ Name = x.SourceName, Link = x.SourceLink }).ToAsyncEnumerable());
}The PVS-Studio warning: V3220 The result of the 'Select' LINQ method with deferred execution is never used. The method will not be executed. TextSearchStore.cs 204
The results variable receives the result of a LINQ method call, but the variable is never used after that. Many LINQ methods have deferred execution. So, value obtained from the LINQ expression is not evaluated when forming that expression. It will be evaluated only when we iterate over the resulting sequence (for example, in a foreach loop).
The value assignment to results is followed by a return statement with a similar LINQ expression. Most likely, one of the LINQ expressions is redundant. The author should either remove the results variable or use it when forming the return value:
return new(results.ToAsyncEnumerable());Code snippet 5
/// <summary>
/// Creates a new instance of the <see cref="KernelProcess"/> class.
/// </summary>
/// <param name="state">The process state.</param>
/// <param name="steps">The steps of the process.</param>
/// <param name="edges">The edges of the process.</param>
/// <param name="threads">The threads associated with the process.</param>
public KernelProcess(
KernelProcessState state,
IList<KernelProcessStepInfo> steps,
Dictionary<string, List<KernelProcessEdge>>? edges = null,
IReadOnlyDictionary<string,
KernelProcessAgentThread>? threads = null)
: base(typeof(KernelProcess), state, edges ?? [])
{
Verify.NotNull(steps);
Verify.NotNullOrWhiteSpace(state.Name);
this.Steps = [.. steps];
}The PVS-Studio warning: V3117 Constructor parameter 'threads' is not used. KernelProcess.cs 46
The threads parameter wasn't used. When calling the KernelProcess method, the caller may pass an argument matching threads. That suggests the caller expects the parameter to be used.
Note that the KernelProcess class has a property named Threads. Perhaps that property should initialize from the unused parameter.
Code snippet 6
private int ComputeTruncationIndex(
IReadOnlyList<ChatMessageContent> chatHistory,
ChatMessageContent? systemMessage)
{
var truncationIndex = -1;
var totalTokenCount = (int)(systemMessage?.Metadata?["TokenCount"] ?? 0);
for (int i = chatHistory.Count - 1; i >= 0; i--) // <=
{
truncationIndex = i;
var tokenCount = (int)(chatHistory[i].Metadata?["TokenCount"] ?? 0);
if (tokenCount + totalTokenCount > this._maxTokenCount)
{
break;
}
totalTokenCount += tokenCount;
}
// Skip function related content
while (truncationIndex < chatHistory.Count) // <=
{
if (chatHistory[truncationIndex].Items.Any(i => i is FunctionCallContent
or FunctionResultContent))
{
truncationIndex++;
}
else
{
break;
}
}
return truncationIndex;
}The PVS-Studio warning: V3106 Possible negative index value. The value of 'truncationIndex' index could reach -1. ChatHistoryMaxTokensReducer.cs 75
The truncationIndex variable initializes to -1. If the chatHistory collection is empty, the execution flow won't enter the for loop as the condition -1 >= 0 fails before the first iteration. Thus, the value of truncationIndex doesn't change. Immediately after the for loop comes a while loop whose condition will be true if chatHistory has zero elements (-1 < 0). Inside the while loop, the code uses truncationIndex as an index, and it may still be -1. Accessing an element with a negative index throws an IndexOutOfRangeException.
To fix this, the developer should check that the index is not negative before using it.
Code snippet 7
public KernelProcessProxy ToKernelProcessProxy()
{
KernelProcessStepInfo processStepInfo = this.ToKernelProcessStepInfo();
if (this.State is not KernelProcessStepState state)
{
throw new KernelException($"Unable to read state from proxy with name " +
$"'{this.State.Name}', Id '{this.State.Id}' " +
$"and type {this.State.GetType()}.");
}
return new KernelProcessProxy(state, this.Edges)
{
ProxyMetadata = this.ProxyMetadata,
};
}
public required KernelProcessStepState State { get; init; }The PVS-Studio warning: V3080 [SEC-NULL] Possible null dereference. Consider inspecting 'this.State'. DaprProxyInfo.cs 30
The condition checks that this.State does not match theKernelProcessStepState type. The State property declaration indicates that its type is KernelProcessStepState. So the condition seems always false? Not exactly. If this.State is null, then this.State is not KernelProcessStepState will be true.
We expect a KernelException in the then block of the if statement. But the actual behavior differes. The exception message accesses properties of this.State. The only way execution flow reaches the then block is when this.State is null, the result will be a NullReferenceException.
Code snippet 8
private static RestApiPayload?
CreateRestApiOperationPayload(string operationId,
OpenApiRequestBody requestBody)
{
if (requestBody?.Content is null)
{
return null;
}
var mediaType = GetMediaType(requestBody.Content)
?? throw new KernelException(
$"Neither of the media types of {operationId} is supported.");
var mediaTypeMetadata = requestBody.Content[mediaType];
var payloadProperties = GetPayloadProperties(operationId,
mediaTypeMetadata.Schema); // <=
return new RestApiPayload(mediaType,
payloadProperties,
requestBody.Description,
mediaTypeMetadata?.Schema?.ToJsonSchema()); // <=
}The PVS-Studio warning: V3095 The 'mediaTypeMetadata' object was used before it was verified against null. Check lines: 464, 466. OpenApiDocumentParser.cs 464
The mediaTypeMetadata variable is fisrt accessed without a null check, and then on the very next line is checked using a null-conditional operator ?.. Perhaps the check in the second case is redundant, and authors should remove it. If mediaTypeMetadata can indeed be null, then the first access will result in a NullReferenceException.
Code snippet 9
internal async Task<ReActStep?> GetNextStepAsync(....)
{
....
if (this._logger.IsEnabled(LogLevel.Information)) // <=
{
this._logger?.LogInformation("question: {Question}", question);
this._logger?.LogInformation("functionDescriptions: {FunctionDescriptions}",
functionDesc);
this._logger?.LogInformation("Scratchpad: {ScratchPad}", scratchPad);
}
var llmResponse = await this._reActFunction
.InvokeAsync(kernel, arguments)
.ConfigureAwait(false);
string llmResponseText = llmResponse.GetValue<string>()!.Trim();
if (this._logger?.IsEnabled(LogLevel.Debug) ?? false)
{
this._logger?.LogDebug("Response : {ActionText}", llmResponseText);
}
....
}The PVS-Studio warning: V3095 The 'this._logger' object was used before it was verified against null. Check lines: 144, 146. ReActEngine.cs 144
This case resembles the previous example. In the same context, the this._loggerfield is used with a null check and without it. Besides the code above, the GetNextStepAsync method contains two more places where this._logger is not checked for null. As before, this indicates inconsistent handling of a potential null value forthis._logger.
Code snippet 10
public override async Task<MAAI.AgentRunResponse>
RunAsync(IEnumerable<ChatMessage> messages,
MAAI.AgentThread? thread = null,
MAAI.AgentRunOptions? options = null,
CancellationToken cancellationToken = default)
{
....
AgentResponseItem<ChatMessageContent>? lastResponseItem = null;
ChatMessage? lastResponseMessage = null; // <=
await foreach (var responseItem in ....)
{
lastResponseItem = responseItem;
}
return new MAAI.AgentRunResponse(responseMessages)
{
AgentId = this._innerAgent.Id,
RawRepresentation = lastResponseItem,
AdditionalProperties = lastResponseMessage?.AdditionalProperties, // <=
CreatedAt = lastResponseMessage?.CreatedAt, // <=
};
}The PVS-Studio warnings:
The lastResponseMessage variable initializes to null. The code then uses its properties to initialize AdditionalProperties and CreatedAt of a new object. Turns out, those properties will always initialize to null.
Perhaps by the time of AdditionalProperties and CreatedAt initialization, lastResponseMessage should contain a non-null value. If the intention is to assign null to these properties, then authors can remove lastResponseMessage, as it only appears in the shown snippets.
Our analysis of the C# portion of Semantic Kernel shows the expected picture: despite the project high profile and Microsoft involvement, the code contains typical issues. This is not unusual—complex systems under active development almost always contain questionable spots and various defects.
If you'd like to analyze your own project with PVS-Studio, you can try the analyzer via this link.
0
0
0
560
1
0