Skip to content

[ALTERNATIVE] Fix UniformBuffer UB regarding UniformType::Enum with extra bits. #3402

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

Closed
wants to merge 2 commits into from
Closed

[ALTERNATIVE] Fix UniformBuffer UB regarding UniformType::Enum with extra bits. #3402

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
9759713
Fix UniformBuffer UB regarding UniformType::Enum with extra bits.
mcourteaux Jan 28, 2025
93bd8c6
Fix assert.
mcourteaux Jan 28, 2025
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
11 changes: 6 additions & 5 deletions src/bgfx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1517,22 +1517,22 @@ namespace bgfx

void UniformBuffer::writeUniform(UniformType::Enum _type, uint16_t _loc, const void* _value, uint16_t _num)
{
const uint32_t opcode = encodeOpcode(_type, _loc, _num, true);
const uint32_t opcode = encodeOpcode(_type, 0, _loc, _num, true);
write(opcode);
write(_value, g_uniformTypeSize[_type]*_num);
}

void UniformBuffer::writeUniformHandle(UniformType::Enum _type, uint16_t _loc, UniformHandle _handle, uint16_t _num)
void UniformBuffer::writeUniformHandle(UniformType::Enum _type, uint8_t _typeBits, uint16_t _loc, UniformHandle _handle, uint16_t _num)
{
const uint32_t opcode = encodeOpcode(_type, _loc, _num, false);
const uint32_t opcode = encodeOpcode(_type, _typeBits, _loc, _num, false);
write(opcode);
write(&_handle, sizeof(UniformHandle) );
}

void UniformBuffer::writeMarker(const bx::StringView& _name)
{
const uint16_t num = bx::narrowCast<uint16_t>(_name.getLength()+1);
const uint32_t opcode = encodeOpcode(bgfx::UniformType::Count, 0, num, true);
const uint32_t opcode = encodeOpcode(bgfx::UniformType::Count, 0, 0, num, true);
write(opcode);
write(_name.getPtr(), num-1);
const char zero = '\0';
Expand Down Expand Up @@ -2521,10 +2521,11 @@ namespace bgfx
}

UniformType::Enum type;
uint8_t typeBits;
uint16_t loc;
uint16_t num;
uint16_t copy;
UniformBuffer::decodeOpcode(opcode, type, loc, num, copy);
UniformBuffer::decodeOpcode(opcode, type, typeBits, loc, num, copy);

const uint32_t size = g_uniformTypeSize[type]*num;
const char* data = _uniformBuffer->read(size);
Expand Down
20 changes: 11 additions & 9 deletions src/bgfx_p.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1508,23 +1508,25 @@ namespace bgfx
}
}

static uint32_t encodeOpcode(UniformType::Enum _type, uint16_t _loc, uint16_t _num, uint16_t _copy)
static uint32_t encodeOpcode(UniformType::Enum _type, uint8_t _typeBits, uint16_t _loc, uint16_t _num, uint16_t _copy)
{
const uint32_t type = _type << kConstantOpcodeTypeShift;
const uint32_t loc = _loc << kConstantOpcodeLocShift;
const uint32_t num = _num << kConstantOpcodeNumShift;
const uint32_t copy = _copy << kConstantOpcodeCopyShift;
const uint32_t type = (_type | _typeBits) << kConstantOpcodeTypeShift;
const uint32_t loc = _loc << kConstantOpcodeLocShift;
const uint32_t num = _num << kConstantOpcodeNumShift;
const uint32_t copy = _copy << kConstantOpcodeCopyShift;
return type|loc|num|copy;
}

static void decodeOpcode(uint32_t _opcode, UniformType::Enum& _type, uint16_t& _loc, uint16_t& _num, uint16_t& _copy)
static void decodeOpcode(uint32_t _opcode, UniformType::Enum& _type, uint8_t& _typeBits, uint16_t& _loc, uint16_t& _num, uint16_t& _copy)
{
const uint32_t type = (_opcode&kConstantOpcodeTypeMask) >> kConstantOpcodeTypeShift;
const uint32_t type = ((_opcode&kConstantOpcodeTypeMask) >> kConstantOpcodeTypeShift) & (~kUniformMask);
const uint32_t bits = ((_opcode&kConstantOpcodeTypeMask) >> kConstantOpcodeTypeShift) & ( kUniformMask);
const uint32_t loc = (_opcode&kConstantOpcodeLocMask ) >> kConstantOpcodeLocShift;
const uint32_t num = (_opcode&kConstantOpcodeNumMask ) >> kConstantOpcodeNumShift;
const uint32_t copy = (_opcode&kConstantOpcodeCopyMask); // >> kConstantOpcodeCopyShift;

_type = (UniformType::Enum)(type);
_type = (UniformType::Enum)type;
_typeBits = (uint8_t)bits;
_copy = (uint16_t)copy;
_num = (uint16_t)num;
_loc = (uint16_t)loc;
Expand Down Expand Up @@ -1583,7 +1585,7 @@ namespace bgfx
}

void writeUniform(UniformType::Enum _type, uint16_t _loc, const void* _value, uint16_t _num = 1);
void writeUniformHandle(UniformType::Enum _type, uint16_t _loc, UniformHandle _handle, uint16_t _num = 1);
void writeUniformHandle(UniformType::Enum _type, uint8_t _typeBits, uint16_t _loc, UniformHandle _handle, uint16_t _num = 1);
void writeMarker(const bx::StringView& _name);

private:
Expand Down
17 changes: 7 additions & 10 deletions src/renderer_d3d11.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3378,10 +3378,11 @@ namespace bgfx { namespace d3d11
}

UniformType::Enum type;
uint8_t typeBits;
uint16_t loc;
uint16_t num;
uint16_t copy;
UniformBuffer::decodeOpcode(opcode, type, loc, num, copy);
UniformBuffer::decodeOpcode(opcode, type, typeBits, loc, num, copy);

const char* data;
if (copy)
Expand All @@ -3395,10 +3396,9 @@ namespace bgfx { namespace d3d11
data = (const char*)m_uniforms[handle.idx];
}

switch ( (uint32_t)type)
switch (type)
{
case UniformType::Mat3:
case UniformType::Mat3|kUniformFragmentBit:
{
float* value = (float*)data;
for (uint32_t ii = 0, count = num/3; ii < count; ++ii, loc += 3*16, value += 9)
Expand All @@ -3416,27 +3416,24 @@ namespace bgfx { namespace d3d11
mtx.un.val[ 9] = value[7];
mtx.un.val[10] = value[8];
mtx.un.val[11] = 0.0f;
setShaderUniform(uint8_t(type), loc, &mtx.un.val[0], 3);
setShaderUniform(uint8_t(type | typeBits), loc, &mtx.un.val[0], 3);
}
}
break;

case UniformType::Sampler:
case UniformType::Sampler | kUniformFragmentBit:
case UniformType::Vec4:
case UniformType::Vec4 | kUniformFragmentBit:
case UniformType::Mat4:
case UniformType::Mat4 | kUniformFragmentBit:
{
setShaderUniform(uint8_t(type), loc, data, num);
setShaderUniform(uint8_t(type | typeBits), loc, data, num);
}
break;

case UniformType::End:
break;

default:
BX_TRACE("%4d: INVALID 0x%08x, t %d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, loc, num, copy);
BX_TRACE("%4d: INVALID 0x%08x, t %d|%d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, typeBits, loc, num, copy);
break;
}
}
Expand Down Expand Up @@ -4190,7 +4187,7 @@ namespace bgfx { namespace d3d11
}

kind = "user";
m_constantBuffer->writeUniformHandle( (UniformType::Enum)(type|fragmentBit), regIndex, info->m_handle, regCount);
m_constantBuffer->writeUniformHandle(UniformType::Enum(type & ~kUniformMask), fragmentBit, regIndex, info->m_handle, regCount);
}
}
else
Expand Down
27 changes: 13 additions & 14 deletions src/renderer_d3d12.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3385,10 +3385,11 @@ namespace bgfx { namespace d3d12
}

UniformType::Enum type;
uint8_t typeBits;
uint16_t loc;
uint16_t num;
uint16_t copy;
UniformBuffer::decodeOpcode(opcode, type, loc, num, copy);
UniformBuffer::decodeOpcode(opcode, type, typeBits, loc, num, copy);

const char* data;
if (copy)
Expand All @@ -3402,10 +3403,9 @@ namespace bgfx { namespace d3d12
data = (const char*)m_uniforms[handle.idx];
}

switch ( (uint32_t)type)
switch (type)
{
case UniformType::Mat3:
case UniformType::Mat3|kUniformFragmentBit:
{
float* value = (float*)data;
for (uint32_t ii = 0, count = num/3; ii < count; ++ii, loc += 3*16, value += 9)
Expand All @@ -3423,27 +3423,24 @@ namespace bgfx { namespace d3d12
mtx.un.val[ 9] = value[7];
mtx.un.val[10] = value[8];
mtx.un.val[11] = 0.0f;
setShaderUniform(uint8_t(type), loc, &mtx.un.val[0], 3);
setShaderUniform(uint8_t(type | typeBits), loc, &mtx.un.val[0], 3);
}
}
break;

case UniformType::Sampler:
case UniformType::Sampler | kUniformFragmentBit:
case UniformType::Vec4:
case UniformType::Vec4 | kUniformFragmentBit:
case UniformType::Mat4:
case UniformType::Mat4 | kUniformFragmentBit:
{
setShaderUniform(uint8_t(type), loc, data, num);
setShaderUniform(uint8_t(type | typeBits), loc, data, num);
}
break;

case UniformType::End:
break;

default:
BX_TRACE("%4d: INVALID 0x%08x, t %d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, loc, num, copy);
BX_TRACE("%4d: INVALID 0x%08x, t %d|%d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, typeBits, loc, num, copy);
break;
}
}
Expand Down Expand Up @@ -4786,8 +4783,10 @@ namespace bgfx { namespace d3d12
bx::read(&reader, &name, nameSize, &err);
name[nameSize] = '\0';

uint8_t type = 0;
bx::read(&reader, type, &err);
uint8_t typeCode = 0;
bx::read(&reader, typeCode, &err);
uint8_t typeBits = typeCode & kUniformMask;
UniformType::Enum type = UniformType::Enum( typeCode & (~kUniformMask) );

uint8_t num = 0;
bx::read(&reader, num, &err);
Expand Down Expand Up @@ -4821,7 +4820,7 @@ namespace bgfx { namespace d3d12
m_predefined[m_numPredefined].m_type = uint8_t(predefined|fragmentBit);
m_numPredefined++;
}
else if (0 == (kUniformSamplerBit & type) )
else if (0 == (kUniformSamplerBit & typeBits) )
{
const UniformRegInfo* info = s_renderD3D12->m_uniformReg.find(name);
BX_WARN(NULL != info, "User defined uniform '%s' is not found, it won't be set.", name);
Expand All @@ -4834,7 +4833,7 @@ namespace bgfx { namespace d3d12
}

kind = "user";
m_constantBuffer->writeUniformHandle( (UniformType::Enum)(type|fragmentBit), regIndex, info->m_handle, regCount);
m_constantBuffer->writeUniformHandle(type, typeBits | fragmentBit, regIndex, info->m_handle, regCount);
}
}
else
Expand All @@ -4845,7 +4844,7 @@ namespace bgfx { namespace d3d12
BX_TRACE("\t%s: %s (%s), num %2d, r.index %3d, r.count %2d"
, kind
, name
, getUniformTypeName(UniformType::Enum(type&~kUniformMask) )
, getUniformTypeName(type)
, num
, regIndex
, regCount
Expand Down
6 changes: 4 additions & 2 deletions src/renderer_gl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4421,10 +4421,12 @@ namespace bgfx { namespace gl
}

UniformType::Enum type;
uint8_t typeBits;
uint16_t ignore;
uint16_t num;
uint16_t copy;
UniformBuffer::decodeOpcode(opcode, type, ignore, num, copy);
UniformBuffer::decodeOpcode(opcode, type, typeBits, ignore, num, copy);
BX_ASSERT(typeBits == 0, "OpenGL never uses typeBits for uniforms.");

const char* data;
if (copy)
Expand Down Expand Up @@ -5313,7 +5315,7 @@ namespace bgfx { namespace gl
}

UniformType::Enum type = convertGlType(gltype);
m_constantBuffer->writeUniformHandle(type, 0, info->m_handle, uint16_t(num) );
m_constantBuffer->writeUniformHandle(type, 0, 0, info->m_handle, uint16_t(num) );
m_constantBuffer->write(loc);
BX_TRACE("store %s %d", name, info->m_handle);
}
Expand Down
17 changes: 7 additions & 10 deletions src/renderer_mtl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1748,10 +1748,11 @@ void commit(UniformBuffer& _uniformBuffer)
}

UniformType::Enum type;
uint8_t typeBits;
uint16_t loc;
uint16_t num;
uint16_t copy;
UniformBuffer::decodeOpcode(opcode, type, loc, num, copy);
UniformBuffer::decodeOpcode(opcode, type, typeBits, loc, num, copy);

const char* data;
if (copy)
Expand All @@ -1765,10 +1766,9 @@ void commit(UniformBuffer& _uniformBuffer)
data = (const char*)m_uniforms[handle.idx];
}

switch ( (uint32_t)type)
switch (type)
{
case UniformType::Mat3:
case UniformType::Mat3|kUniformFragmentBit:
{
float* value = (float*)data;
for (uint32_t ii = 0, count = num/3; ii < count; ++ii, loc += 3*16, value += 9)
Expand All @@ -1786,27 +1786,24 @@ void commit(UniformBuffer& _uniformBuffer)
mtx.un.val[ 9] = value[7];
mtx.un.val[10] = value[8];
mtx.un.val[11] = 0.0f;
setShaderUniform(uint8_t(type), loc, &mtx.un.val[0], 3);
setShaderUniform(uint8_t(type | typeBits), loc, &mtx.un.val[0], 3);
}
}
break;

case UniformType::Sampler:
case UniformType::Sampler | kUniformFragmentBit:
case UniformType::Vec4:
case UniformType::Vec4 | kUniformFragmentBit:
case UniformType::Mat4:
case UniformType::Mat4 | kUniformFragmentBit:
{
setShaderUniform(uint8_t(type), loc, data, num);
setShaderUniform(uint8_t(type | typeBits), loc, data, num);
}
break;

case UniformType::End:
break;

default:
BX_TRACE("%4d: INVALID 0x%08x, t %d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, loc, num, copy);
BX_TRACE("%4d: INVALID 0x%08x, t %d|%d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, typeBits, loc, num, copy);
break;
}
}
Expand Down Expand Up @@ -2225,7 +2222,7 @@ void processArguments(
}

UniformType::Enum type = convertMtlType(dataType);
constantBuffer->writeUniformHandle( (UniformType::Enum)(type|fragmentBit), uint32_t(uniform.offset), info->m_handle, uint16_t(num) );
constantBuffer->writeUniformHandle(type, fragmentBit, uint32_t(uniform.offset), info->m_handle, uint16_t(num) );
BX_TRACE("store %s %d offset:%d", name, info->m_handle, uint32_t(uniform.offset) );
}
}
Expand Down
17 changes: 7 additions & 10 deletions src/renderer_vk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4151,10 +4151,11 @@ VK_IMPORT_DEVICE
}

UniformType::Enum type;
uint8_t typeBits;
uint16_t loc;
uint16_t num;
uint16_t copy;
UniformBuffer::decodeOpcode(opcode, type, loc, num, copy);
UniformBuffer::decodeOpcode(opcode, type, typeBits, loc, num, copy);

const char* data;
if (copy)
Expand All @@ -4168,10 +4169,9 @@ VK_IMPORT_DEVICE
data = (const char*)m_uniforms[handle.idx];
}

switch ( (uint32_t)type)
switch (type)
{
case UniformType::Mat3:
case UniformType::Mat3|kUniformFragmentBit:
{
float* value = (float*)data;
for (uint32_t ii = 0, count = num/3; ii < count; ++ii, loc += 3*16, value += 9)
Expand All @@ -4189,30 +4189,27 @@ VK_IMPORT_DEVICE
mtx.un.val[ 9] = value[7];
mtx.un.val[10] = value[8];
mtx.un.val[11] = 0.0f;
setShaderUniform(uint8_t(type), loc, &mtx.un.val[0], 3);
setShaderUniform(uint8_t(type | typeBits), loc, &mtx.un.val[0], 3);
}
}
break;

case UniformType::Sampler:
case UniformType::Sampler|kUniformFragmentBit:
// do nothing, but VkDescriptorSetImageInfo would be set before drawing
break;

case UniformType::Vec4:
case UniformType::Vec4 | kUniformFragmentBit:
case UniformType::Mat4:
case UniformType::Mat4 | kUniformFragmentBit:
{
setShaderUniform(uint8_t(type), loc, data, num);
setShaderUniform(uint8_t(type | typeBits), loc, data, num);
}
break;

case UniformType::End:
break;

default:
BX_TRACE("%4d: INVALID 0x%08x, t %d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, loc, num, copy);
BX_TRACE("%4d: INVALID 0x%08x, t %d|%d, l %d, n %d, c %d", _uniformBuffer.getPos(), opcode, type, typeBits, loc, num, copy);
break;
}
}
Expand Down Expand Up @@ -5090,7 +5087,7 @@ VK_DESTROY
}

kind = "user";
m_constantBuffer->writeUniformHandle( (UniformType::Enum)(type|fragmentBit), regIndex, info->m_handle, regCount);
m_constantBuffer->writeUniformHandle(UniformType::Enum(type & ~kUniformMask), fragmentBit, regIndex, info->m_handle, regCount);
}
}
}
Expand Down