Мы используем куки, чтобы пользоваться сайтом
было удобно.
Вебинар: Подводные камни регулярных выражений: катастрофический возврат, ReDoS-атаки и выявление уязвимостей - 30.04
Semantic Kernel — SDK от Microsoft для интеграции моделей искусственного интеллекта в приложения. Сможет ли статический анализатор PVS-Studio отыскать дефекты в исходниках такого проекта? Постараемся ответить на этот вопрос в статье. Приятного чтения!
Проекты, связанные с интеграцией искусственного интеллекта, всё чаще становятся частью повседневной разработки. Один из таких проектов — Semantic Kernel. Он представляет собой SDK для построения AI-агентов и оркестрации LLM-сценариев и активно развивается компанией Microsoft.
Однако под капотом даже самых современных решений скрывается вполне обычный C# код — со всеми присущими ему проблемами. В этом мы с вами сегодня постараемся убедиться.
Для проверки С# части проекта использовался статический анализатор PVS-Studio версии 7.41. Проверяемый исходный код соответствует коммиту.
Стоит отметить, что мы расскажем только о самых интересных для рассмотрения фрагментах кода, чтобы не затягивать статью.
Приступим к разбору подозрительных мест!
Фрагмент кода 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)
}
....
}Предупреждение PVS-Studio: 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
Скорее всего, предполагалась, что условие будет работать так: если knowledgeBase.Options?.TryGetValue(KnowledgeBaseId, out var value) не null, то проверяется, что value is not null && value is string, однако поведение будет иным. Оператор && имеет более высокий приоритет, чем ??, поэтому проверки для value не будут выполнены.
Ещё один маркер того, что здесь допущена ошибка — правый операнд ??. Он имеет вид: false && value is not null && value is string. Нетрудно догадаться, каков будет результат его выполнения. Условие содержит только операторы &&, а один из операндов — false. Следовательно, всё выражение тоже false.
Для исправления нужно добавить скобки:
(knowledgeBase.Options
?.TryGetValue(KnowledgeBaseId,
out var value) ?? false) && value is not null
&& value is stringСтоит отметить, что есть ещё 4 места в исходном коде, которые содержат аналогичную проблему:
Фрагмент кода 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;
};Предупреждение PVS-Studio: V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. CopilotAgentBasedPlugins.cs 212
Скобки верхнего уровня полностью обрамляют условие if, что делает их бесполезными. Возможно, предполагалась следующая логика: применять проверку имени параметра payload для обеих операций (me_sendMail и me_calendar_CreateEvents). Однако фактически проверка payload выполняется только для второй операции, так как приоритет && выше, чем у ||. Для исправления нужно взять в скобки только проверки для me_sendMail и me_calendar_CreateEvents, а не всё выражение условия целиком.
Фрагмент кода 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'."
);
}Предупреждение PVS-Studio: 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
В методе GetAudioOutputMimeType дважды встречается одинаковая проверка: audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav. Повторная проверка на соответствие ChatOutputAudioFormat.Wav является недостижимым кодом — при первом совпадении выполнение метода завершится. Это типичная ошибка, которая может быть допущена при механическом копировании.
Что примечательно, в структуре ChatOutputAudioFormat объявлен формат ChatOutputAudioFormat Aac. Это единственный формат, который не обрабатывается в методе GetAudioOutputMimeType. Возможно, именно он должен быть вместо одной из проверок audioOptions.OutputAudioFormat == ChatOutputAudioFormat.Wav.
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);
....
}Фрагмент кода 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());
}Предупреждение PVS-Studio: V3220 The result of the 'Select' LINQ method with deferred execution is never used. The method will not be executed. TextSearchStore.cs 204
Переменной results присваивается результат вызова LINQ метода, после чего переменная не используется. Многие LINQ методы имеют отложенное выполнение. Соответственно, вычисление значения, получаемого из LINQ выражения, не осуществляется при формировании этого выражения. Оно будет вычислено при обходе полученной последовательности (например, в foreach).
Сразу после присваивания значения переменной results идёт return с аналогичным LINQ выражением. Скорее всего, одно из LINQ выражений лишнее. Нужно либо вообще убрать переменную results, либо использовать её при формировании возвращаемого значения:
return new(results.ToAsyncEnumerable());Фрагмент кода 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];
}Предупреждение PVS-Studio: V3117 Constructor parameter 'threads' is not used. KernelProcess.cs 46
Параметр threads не был использован. При некоторых вызовах метода KernelProcess передаётся аргумент, соответствующий параметру threads. Получается, что вызывающий код рассчитывает на использование этого параметра.
Стоит отметить, что в классе KernelProcess есть свойство Threads, возможно, именно оно должно инициализироваться неиспользуемым параметром.
Фрагмент кода 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;
}Предупреждение PVS-Studio: V3106 Possible negative index value. The value of 'truncationIndex' index could reach -1. ChatHistoryMaxTokensReducer.cs 75
Переменная truncationIndex инициализируется значением -1. Если коллекция chatHistory будет пустой, то поток выполнения не зайдёт в цикл for, так как перед первой итерацией выполнится проверка -1 >= 0. Следовательно, значение для truncationIndex не поменяется. Сразу после for идёт цикл while, условие которого будет истинным, если в chatHistory ноль элементов (-1 < 0). В теле while в качестве индекса используется переменная truncationIndex, которая может иметь значение -1. При обращении по отрицательному индексу будет выброшено исключение IndexOutOfRangeException.
Для исправления нужно проверять, что индекс не отрицательный, перед обращением по нему.
Фрагмент кода 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; }Предупреждение PVS-Studio: V3080 [SEC-NULL] Possible null dereference. Consider inspecting 'this.State'. DaprProxyInfo.cs 30
В условии проверяется, что this.State не соответствует типу KernelProcessStepState. Если посмотреть на объявление свойства State, можно увидеть, что оно имеет тип KernelProcessStepState. Получается, что условие всегда ложное? На самом деле это не совсем так. Если this.State будет иметь значение null, то this.State is not KernelProcessStepState — true.
Ожидается, что в блоке then оператора if будет выброшено исключение типа KernelException, однако поведение будет иным. Так как в сообщении исключения обращаются к свойствам this.State, а единственное условие, при котором поток выполнения зайдёт в then блок this.State — null, будет выброшено исключение NullReferenceException.
Фрагмент кода 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()); // <=
}Предупреждение PVS-Studio: V3095 The 'mediaTypeMetadata' object was used before it was verified against null. Check lines: 464, 466. OpenApiDocumentParser.cs 464
К переменной mediaTypeMetadata сначала обращаются без проверки на null, а буквально на соседней строчке с проверкой ?.. Возможно, проверка во втором случае избыточна, тогда её стоит убрать. Если же mediaTypeMetadata может иметь значение null, то при первом обращении будет выброшено исключение NullReferenceException.
Фрагмент кода 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);
}
....
}Предупреждение PVS-Studio: V3095 The 'this._logger' object was used before it was verified against null. Check lines: 144, 146. ReActEngine.cs 144
Ситуация схожа с той, которая была в предыдущем примере. Поле this._logger в одном и том же контексте используется то с проверкой, то без неё. Стоит отметить, что помимо представленного кода, в методе GetNextStepAsync есть ещё два места, где this._logger не проверяется на null. Как и в предыдущем примере, здесь присутствует проблема согласованности возможного значения null для this._logger.
Фрагмент кода 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, // <=
};
}Предупреждения PVS-Studio:
Переменной lastResponseMessage присваивается значение null. С помощью свойств этой переменной инициализируются AdditionalProperties и CreatedAt нового объекта. Получается, что они всегда будут инициализироваться значением null.
Возможно, на момент инициализации AdditionalProperties и CreatedAt переменная lastResponseMessage должна иметь значение, отличное от null. Если же свойства должны инициализироваться значением null, то lastResponseMessage нужно убрать, так как она фигурирует только в рассматриваемых местах.
Проверка C# части Semantic Kernel показала ожидаемую картину: несмотря на высокий уровень проекта и участие крупной компании, код не лишён типичных проблем. Это не является чем-то необычным — сложные системы с активной разработкой почти всегда содержат спорные места и различные дефекты.
Если вы хотите самостоятельно проверить проект с помощью PVS-Studio, то попробовать анализатор можете по ссылке!
0
0
0
125
0
0