Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add memory bounds-check when expanding points, rects and lines to triangles #18184

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Add memory bounds-check when expanding points, rects and lines to triangles #18184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f2ef50
Make it easier to reason about space in the inds buffer by moving an …
hrydgard May 2, 2023
966144f
Bounds check writing to the index buffer when expanding lines/rects/p…
hrydgard May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 56 additions & 19 deletions GPU/Common/SoftwareTransformCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ void SoftwareTransform::DetectOffsetTexture(int maxIndex) {
}

// NOTE: The viewport must be up to date!
void SoftwareTransform::BuildDrawingParams(int prim, int vertexCount, u32 vertType, u16 *&inds, int &maxIndex, SoftwareTransformResult *result) {
void SoftwareTransform::BuildDrawingParams(int prim, int vertexCount, u32 vertType, u16 *inds, int &indsOffset, int indexBufferSize, int &maxIndex, SoftwareTransformResult *result) {
TransformedVertex *transformed = params_.transformed;
TransformedVertex *transformedExpanded = params_.transformedExpanded;
bool throughmode = (vertType & GE_VTYPE_THROUGH_MASK) != 0;
Expand All @@ -560,7 +560,11 @@ void SoftwareTransform::BuildDrawingParams(int prim, int vertexCount, u32 vertTy
bool useBufferedRendering = fbman->UseBufferedRendering();

if (prim == GE_PRIM_RECTANGLES) {
ExpandRectangles(vertexCount, maxIndex, inds, transformed, transformedExpanded, numTrans, throughmode);
if (!ExpandRectangles(vertexCount, maxIndex, inds, indsOffset, indexBufferSize, transformed, transformedExpanded, numTrans, throughmode)) {
result->drawIndexed = false;
result->drawNumTrans = 0;
return;
}
result->drawBuffer = transformedExpanded;
result->drawIndexed = true;

Expand All @@ -578,11 +582,19 @@ void SoftwareTransform::BuildDrawingParams(int prim, int vertexCount, u32 vertTy
}
}
} else if (prim == GE_PRIM_POINTS) {
ExpandPoints(vertexCount, maxIndex, inds, transformed, transformedExpanded, numTrans, throughmode);
if (!ExpandPoints(vertexCount, maxIndex, inds, indsOffset, indexBufferSize, transformed, transformedExpanded, numTrans, throughmode)) {
result->drawIndexed = false;
result->drawNumTrans = 0;
return;
}
result->drawBuffer = transformedExpanded;
result->drawIndexed = true;
} else if (prim == GE_PRIM_LINES) {
ExpandLines(vertexCount, maxIndex, inds, transformed, transformedExpanded, numTrans, throughmode);
if (!ExpandLines(vertexCount, maxIndex, inds, indsOffset, indexBufferSize, transformed, transformedExpanded, numTrans, throughmode)) {
result->drawIndexed = false;
result->drawNumTrans = 0;
return;
}
result->drawBuffer = transformedExpanded;
result->drawIndexed = true;
} else {
Expand Down Expand Up @@ -674,15 +686,21 @@ void SoftwareTransform::CalcCullParams(float &minZValue, float &maxZValue) {
std::swap(minZValue, maxZValue);
}

void SoftwareTransform::ExpandRectangles(int vertexCount, int &maxIndex, u16 *&inds, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
bool SoftwareTransform::ExpandRectangles(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
// Before we start, do a sanity check - does the output fit?
if ((vertexCount / 2) * 6 > indexBufferSize - indsOffset) {
// Won't fit, kill the draw.
return false;
}

// Rectangles always need 2 vertices, disregard the last one if there's an odd number.
vertexCount = vertexCount & ~1;
numTrans = 0;
TransformedVertex *trans = &transformedExpanded[0];

const u16 *indsIn = (const u16 *)inds;
u16 *newInds = inds + vertexCount;
u16 *indsOut = newInds;
const u16 *indsIn = (const u16 *)(inds + indsOffset);
int newIndsOffset = indsOffset + vertexCount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this not add enough space to not overlap? Not sure if maybe it's still safe. Before it was (u16 *)x + y aka x + y * 2 and now it's x + y. Also in other places here.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh duh, sloppy of me to miss. My test cases still worked somehow though..

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually hm, indsOffset is in 16-bit units here, since we just after add it to a u16*. But I think there might be some unclarity here, will check it all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought one of these had been changed to a u8 because of the cast, I guess. My mistake.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, something was indeed wrong and I had to revert, but maybe not here. Will redo this later.

u16 *indsOut = inds + newIndsOffset;

maxIndex = 4 * (vertexCount / 2);
for (int i = 0; i < vertexCount; i += 2) {
Expand Down Expand Up @@ -727,23 +745,33 @@ void SoftwareTransform::ExpandRectangles(int vertexCount, int &maxIndex, u16 *&i
indsOut[3] = i * 2 + 3;
indsOut[4] = i * 2 + 0;
indsOut[5] = i * 2 + 2;

trans += 4;
indsOut += 6;

numTrans += 6;
}
inds = newInds;

indsOffset = newIndsOffset;
return true;
}

void SoftwareTransform::ExpandLines(int vertexCount, int &maxIndex, u16 *&inds, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
bool SoftwareTransform::ExpandLines(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
// Before we start, do a sanity check - does the output fit?
if ((vertexCount / 2) * 6 > indexBufferSize - indsOffset) {
// Won't fit, kill the draw.
return false;
}

// Lines always need 2 vertices, disregard the last one if there's an odd number.
vertexCount = vertexCount & ~1;
numTrans = 0;
TransformedVertex *trans = &transformedExpanded[0];

const u16 *indsIn = (const u16 *)inds;
u16 *newInds = inds + vertexCount;
u16 *indsOut = newInds;

const u16 *indsIn = (const u16 *)(inds + indsOffset);
int newIndsOffset = indsOffset + vertexCount;
u16 *indsOut = inds + newIndsOffset;

float dx = 1.0f * gstate_c.vpWidthScale * (1.0f / fabsf(gstate.getViewportXScale()));
float dy = 1.0f * gstate_c.vpHeightScale * (1.0f / fabsf(gstate.getViewportYScale()));
Expand Down Expand Up @@ -856,16 +884,23 @@ void SoftwareTransform::ExpandLines(int vertexCount, int &maxIndex, u16 *&inds,
}
}

inds = newInds;
indsOffset = newIndsOffset;
return true;
}

void SoftwareTransform::ExpandPoints(int vertexCount, int &maxIndex, u16 *&inds, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
bool SoftwareTransform::ExpandPoints(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
// Before we start, do a sanity check - does the output fit?
if (vertexCount * 6 > indexBufferSize - indsOffset) {
// Won't fit, kill the draw.
return false;
}

numTrans = 0;
TransformedVertex *trans = &transformedExpanded[0];

const u16 *indsIn = (const u16 *)inds;
u16 *newInds = inds + vertexCount;
u16 *indsOut = newInds;
const u16 *indsIn = (const u16 *)(inds + indsOffset);
int newIndsOffset = indsOffset + vertexCount;
u16 *indsOut = inds + newIndsOffset;

float dx = 1.0f * gstate_c.vpWidthScale * (1.0f / gstate.getViewportXScale());
float dy = 1.0f * gstate_c.vpHeightScale * (1.0f / gstate.getViewportYScale());
Expand Down Expand Up @@ -924,5 +959,7 @@ void SoftwareTransform::ExpandPoints(int vertexCount, int &maxIndex, u16 *&inds,

numTrans += 6;
}
inds = newInds;

indsOffset = newIndsOffset;
return true;
}
11 changes: 5 additions & 6 deletions GPU/Common/SoftwareTransformCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,18 @@ struct SoftwareTransformParams {

class SoftwareTransform {
public:
SoftwareTransform(SoftwareTransformParams &params) : params_(params) {
}
SoftwareTransform(SoftwareTransformParams &params) : params_(params) {}

void SetProjMatrix(const float mtx[14], bool invertedX, bool invertedY, const Lin::Vec3 &trans, const Lin::Vec3 &scale);
void Decode(int prim, u32 vertexType, const DecVtxFormat &decVtxFormat, int maxIndex, SoftwareTransformResult *result);
void DetectOffsetTexture(int maxIndex);
void BuildDrawingParams(int prim, int vertexCount, u32 vertType, u16 *&inds, int &maxIndex, SoftwareTransformResult *result);
void BuildDrawingParams(int prim, int vertexCount, u32 vertType, u16 *inds, int &indsOffset, int indexBufferSize, int &maxIndex, SoftwareTransformResult *result);

protected:
void CalcCullParams(float &minZValue, float &maxZValue);
void ExpandRectangles(int vertexCount, int &maxIndex, u16 *&inds, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode);
void ExpandLines(int vertexCount, int &maxIndex, u16 *&inds, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode);
void ExpandPoints(int vertexCount, int &maxIndex, u16 *&inds, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode);
bool ExpandRectangles(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode);
bool ExpandLines(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode);
bool ExpandPoints(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode);

const SoftwareTransformParams &params_;
Lin::Matrix4x4 projMatrix_;
Expand Down
9 changes: 5 additions & 4 deletions GPU/D3D11/DrawEngineD3D11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,7 @@ void DrawEngineD3D11::DoFlush() {
prim = GE_PRIM_TRIANGLES;
VERBOSE_LOG(G3D, "Flush prim %i SW! %i verts in one go", prim, indexGen.VertexCount());

u16 *inds = decIndex_;
u16 *const inds = decIndex_;
SoftwareTransformResult result{};
SoftwareTransformParams params{};
params.decoded = decoded_;
Expand Down Expand Up @@ -644,8 +644,9 @@ void DrawEngineD3D11::DoFlush() {
// Need to ApplyDrawState after ApplyTexture because depal can launch a render pass and that wrecks the state.
ApplyDrawState(prim);

int indsOffset = 0;
if (result.action == SW_NOT_READY)
swTransform.BuildDrawingParams(prim, indexGen.VertexCount(), dec_->VertexType(), inds, maxIndex, &result);
swTransform.BuildDrawingParams(prim, indexGen.VertexCount(), dec_->VertexType(), inds, indsOffset, DECODED_INDEX_BUFFER_SIZE / sizeof(uint16_t), maxIndex, &result);
if (result.setSafeSize)
framebufferManager_->SetSafeSize(result.safeWidth, result.safeHeight);

Expand Down Expand Up @@ -683,11 +684,11 @@ void DrawEngineD3D11::DoFlush() {
UINT iOffset;
int iSize = sizeof(uint16_t) * result.drawNumTrans;
uint8_t *iptr = pushInds_->BeginPush(context_, &iOffset, iSize);
memcpy(iptr, inds, iSize);
memcpy(iptr, inds + indsOffset, iSize);
pushInds_->EndPush(context_);
context_->IASetIndexBuffer(pushInds_->Buf(), DXGI_FORMAT_R16_UINT, iOffset);
context_->DrawIndexed(result.drawNumTrans, 0, 0);
} else {
} else if (result.drawNumTrans > 0) {
context_->Draw(result.drawNumTrans, 0);
}
} else if (result.action == SW_CLEAR) {
Expand Down
9 changes: 5 additions & 4 deletions GPU/Directx9/DrawEngineDX9.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ void DrawEngineDX9::DoFlush() {
prim = GE_PRIM_TRIANGLES;
VERBOSE_LOG(G3D, "Flush prim %i SW! %i verts in one go", prim, indexGen.VertexCount());

u16 *inds = decIndex_;
u16 *const inds = decIndex_;
SoftwareTransformResult result{};
SoftwareTransformParams params{};
params.decoded = decoded_;
Expand Down Expand Up @@ -607,8 +607,9 @@ void DrawEngineDX9::DoFlush() {

ApplyDrawState(prim);

int indsOffset = 0;
if (result.action == SW_NOT_READY)
swTransform.BuildDrawingParams(prim, indexGen.VertexCount(), dec_->VertexType(), inds, maxIndex, &result);
swTransform.BuildDrawingParams(prim, indexGen.VertexCount(), dec_->VertexType(), inds, indsOffset, DECODED_INDEX_BUFFER_SIZE / sizeof(uint16_t), maxIndex, &result);
if (result.setSafeSize)
framebufferManager_->SetSafeSize(result.safeWidth, result.safeHeight);

Expand All @@ -628,8 +629,8 @@ void DrawEngineDX9::DoFlush() {

device_->SetVertexDeclaration(transformedVertexDecl_);
if (result.drawIndexed) {
device_->DrawIndexedPrimitiveUP(d3d_prim[prim], 0, maxIndex, D3DPrimCount(d3d_prim[prim], result.drawNumTrans), inds, D3DFMT_INDEX16, result.drawBuffer, sizeof(TransformedVertex));
} else {
device_->DrawIndexedPrimitiveUP(d3d_prim[prim], 0, maxIndex, D3DPrimCount(d3d_prim[prim], result.drawNumTrans), inds + indsOffset, D3DFMT_INDEX16, result.drawBuffer, sizeof(TransformedVertex));
} else if (result.drawNumTrans > 0) {
device_->DrawPrimitiveUP(d3d_prim[prim], D3DPrimCount(d3d_prim[prim], result.drawNumTrans), result.drawBuffer, sizeof(TransformedVertex));
}
} else if (result.action == SW_CLEAR) {
Expand Down
9 changes: 5 additions & 4 deletions GPU/GLES/DrawEngineGLES.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ void DrawEngineGLES::DoFlush() {
if (prim == GE_PRIM_TRIANGLE_STRIP)
prim = GE_PRIM_TRIANGLES;

u16 *inds = decIndex_;
u16 *const inds = decIndex_;
SoftwareTransformResult result{};
// TODO: Keep this static? Faster than repopulating?
SoftwareTransformParams params{};
Expand Down Expand Up @@ -414,8 +414,9 @@ void DrawEngineGLES::DoFlush() {
// Need to ApplyDrawState after ApplyTexture because depal can launch a render pass and that wrecks the state.
ApplyDrawState(prim);

int indsOffset = 0;
if (result.action == SW_NOT_READY)
swTransform.BuildDrawingParams(prim, vertexCount, dec_->VertexType(), inds, maxIndex, &result);
swTransform.BuildDrawingParams(prim, vertexCount, dec_->VertexType(), inds, indsOffset, DECODED_INDEX_BUFFER_SIZE / sizeof(uint16_t), maxIndex, &result);
if (result.setSafeSize)
framebufferManager_->SetSafeSize(result.safeWidth, result.safeHeight);

Expand All @@ -430,11 +431,11 @@ void DrawEngineGLES::DoFlush() {
if (result.action == SW_DRAW_PRIMITIVES) {
if (result.drawIndexed) {
vertexBufferOffset = (uint32_t)frameData.pushVertex->Push(result.drawBuffer, maxIndex * sizeof(TransformedVertex), 4, &vertexBuffer);
indexBufferOffset = (uint32_t)frameData.pushIndex->Push(inds, sizeof(uint16_t) * result.drawNumTrans, 2, &indexBuffer);
indexBufferOffset = (uint32_t)frameData.pushIndex->Push(inds + indsOffset, sizeof(uint16_t) * result.drawNumTrans, 2, &indexBuffer);
render_->DrawIndexed(
softwareInputLayout_, vertexBuffer, vertexBufferOffset, indexBuffer, indexBufferOffset,
glprim[prim], result.drawNumTrans, GL_UNSIGNED_SHORT);
} else {
} else if (result.drawNumTrans > 0) {
vertexBufferOffset = (uint32_t)frameData.pushVertex->Push(result.drawBuffer, result.drawNumTrans * sizeof(TransformedVertex), 4, &vertexBuffer);
render_->Draw(
softwareInputLayout_, vertexBuffer, vertexBufferOffset, glprim[prim], 0, result.drawNumTrans);
Expand Down
9 changes: 5 additions & 4 deletions GPU/Vulkan/DrawEngineVulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,7 @@ void DrawEngineVulkan::DoFlush() {
if (prim == GE_PRIM_TRIANGLE_STRIP)
prim = GE_PRIM_TRIANGLES;

u16 *inds = decIndex_;
u16 *const inds = decIndex_;
SoftwareTransformResult result{};
SoftwareTransformParams params{};
params.decoded = decoded_;
Expand Down Expand Up @@ -898,9 +898,10 @@ void DrawEngineVulkan::DoFlush() {
// Games sometimes expect exact matches (see #12626, for example) for equal comparisons.
if (result.action == SW_CLEAR && everUsedEqualDepth_ && gstate.isClearModeDepthMask() && result.depth > 0.0f && result.depth < 1.0f)
result.action = SW_NOT_READY;
int indsOffset = 0;
if (result.action == SW_NOT_READY) {
swTransform.DetectOffsetTexture(maxIndex);
swTransform.BuildDrawingParams(prim, indexGen.VertexCount(), dec_->VertexType(), inds, maxIndex, &result);
swTransform.BuildDrawingParams(prim, indexGen.VertexCount(), dec_->VertexType(), inds, indsOffset, DECODED_INDEX_BUFFER_SIZE / sizeof(uint16_t), maxIndex, &result);
}

if (result.setSafeSize)
Expand Down Expand Up @@ -970,9 +971,9 @@ void DrawEngineVulkan::DoFlush() {
if (result.drawIndexed) {
VkBuffer vbuf, ibuf;
vbOffset = (uint32_t)pushVertex_->Push(result.drawBuffer, maxIndex * sizeof(TransformedVertex), 4, &vbuf);
ibOffset = (uint32_t)pushIndex_->Push(inds, sizeof(short) * result.drawNumTrans, 4, &ibuf);
ibOffset = (uint32_t)pushIndex_->Push(inds + indsOffset, sizeof(short) * result.drawNumTrans, 4, &ibuf);
renderManager->DrawIndexed(ds, ARRAY_SIZE(dynamicUBOOffsets), dynamicUBOOffsets, vbuf, vbOffset, ibuf, ibOffset, result.drawNumTrans, 1);
} else {
} else if (result.drawNumTrans > 0) {
VkBuffer vbuf;
vbOffset = (uint32_t)pushVertex_->Push(result.drawBuffer, result.drawNumTrans * sizeof(TransformedVertex), 4, &vbuf);
renderManager->Draw(ds, ARRAY_SIZE(dynamicUBOOffsets), dynamicUBOOffsets, vbuf, vbOffset, result.drawNumTrans);
Expand Down