Skip to content

Commit

Permalink
Fix UniformBuffer UB regarding UniformType::Enum with extra bits.
Browse files Browse the repository at this point in the history
  • Loading branch information
@mcourteaux
mcourteaux committed Jan 28, 2025
1 parent 68a6788 commit 9759713
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 60 deletions.
11 changes: 6 additions & 5 deletions src/bgfx.cpp
View file
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
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
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
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
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);

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
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
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

0 comments on commit 9759713

Please sign in to comment.