BeRo1985 / pasvulkan

Vulkan header generator, OOP-style API wrapper, framework and prospective Vulkan-based game engine for Object Pascal
zlib License
191 stars 30 forks source link

Dangerous SetLength call #27

Closed johnhutch111 closed 1 year ago

johnhutch111 commented 1 year ago

In the TpvVulkanFrameBuffer.Initialize procedure (PasVulkan.Framework) a call to set the length of the fFrameBufferAttachments is made. This should already be set previously by the Add.. calls made previously. If the length is not correct here, changing it will mess with the already set FrameBufferAttachments links. As is, the setLength does nothing so doesn't break any links but this seems to be a dangerous call.

procedure TpvVulkanFrameBuffer.Initialize; var Index:TpvInt32; FrameBufferCreateInfo:TVkFramebufferCreateInfo; begin if fFrameBufferHandle=VK_NULL_HANDLE then begin

SetLength(fFrameBufferAttachments,fCountFrameBufferAttachments);

SetLength(fFrameBufferAttachmentImageViews,fCountFrameBufferAttachments);

for Index:=0 to fCountFrameBufferAttachments-1 do begin fFrameBufferAttachmentImageViews[Index]:=fFrameBufferAttachments[Index].fImageView.fImageViewHandle; end;

FillChar(FrameBufferCreateInfo,SizeOf(TVkFramebufferCreateInfo),#0); FrameBufferCreateInfo.sType:=VK_STRUCTURE_TYPE_FRAMEBUFFER_CREATE_INFO; FrameBufferCreateInfo.pNext:=nil; FrameBufferCreateInfo.flags:=0; FrameBufferCreateInfo.renderPass:=fRenderPass.fRenderPassHandle; FrameBufferCreateInfo.attachmentCount:=fCountFrameBufferAttachments; FrameBufferCreateInfo.pAttachments:=@fFrameBufferAttachmentImageViews[0]; FrameBufferCreateInfo.width:=fWidth; FrameBufferCreateInfo.height:=fHeight; FrameBufferCreateInfo.layers:=fLayers;

VulkanCheckResult(fDevice.fDeviceVulkan.CreateFramebuffer(fDevice.fDeviceHandle,@FrameBufferCreateInfo,fDevice.fAllocationCallbacks,@fFrameBufferHandle));

end; end;

BeRo1985 commented 1 year ago

No, because

function TpvVulkanFrameBuffer.AddAttachment(const aFrameBufferAttachment:TpvVulkanFrameBufferAttachment):TpvInt32;
begin
 result:=fCountFrameBufferAttachments;
 inc(fCountFrameBufferAttachments);
 if fCountFrameBufferAttachments>length(fFrameBufferAttachments) then begin
  SetLength(fFrameBufferAttachments,fCountFrameBufferAttachments*2);
 end;
 fFrameBufferAttachments[result]:=aFrameBufferAttachment;
end; 

resizes fFrameBufferAttachments in advance by factor 2 (or as alternatve by factor 1.5) to save multiple resizing in succession. It is a common memory capacity pre-allocation strategy of linear allocators / growing of dynamic arrays, and the SetLength inside pvVulkanFrameBuffer.Initialize sets the size of fFrameBufferAttachments just again down to the real used count of frame buffer attachment items without the preallocated in-advance-factor-2x area.

Therefore, my request for the future, read the code more carefully, and above all try to understand the overall context.

johnhutch111 commented 1 year ago

Roger that