From 170331b105e83ba25b0041a743ca429cd1a5325f Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Fri, 21 Feb 2020 18:46:32 -0700 Subject: [PATCH] C++: Better fix for `void` type on buffer access Fixes issue https://github.com/github/codeql-c-analysis-team/issues/20 This change undoes the workaround in https://github.com/Semmle/ql/pull/2736, and replaces it with a fix for the underlying cause. The problem was that the IR construction code for side effects incorrectly assumed that `BufferAccessOpcode` included `SizedBufferAccessOpcode`. I think that was actually a perfectly reasonable assumption to make, so I changed the `Opcode` hierarchy to make it true. --- .../code/cpp/ir/implementation/Opcode.qll | 22 +++++++++++-------- .../aliased_ssa/internal/AliasedSSA.qll | 4 ++-- .../raw/internal/TranslatedCall.qll | 2 +- .../code/csharp/ir/implementation/Opcode.qll | 22 +++++++++++-------- 4 files changed, 29 insertions(+), 21 deletions(-) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll index 886183c32ba..3fb716d78e0 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll @@ -213,23 +213,29 @@ abstract class IndirectReadOpcode extends IndirectMemoryAccessOpcode { } /** - * An opcode that accesses a memory buffer of unknown size. + * An opcode that accesses a memory buffer. */ abstract class BufferAccessOpcode extends Opcode { final override predicate hasAddressOperand() { any() } } +/** + * An opcode that accesses a memory buffer of unknown size. + */ +abstract class UnsizedBufferAccessOpcode extends BufferAccessOpcode { +} + /** * An opcode that writes to a memory buffer of unknown size. */ -abstract class BufferWriteOpcode extends BufferAccessOpcode { +abstract class UnsizedBufferWriteOpcode extends UnsizedBufferAccessOpcode { final override MemoryAccessKind getWriteMemoryAccess() { result instanceof BufferMemoryAccess } } /** * An opcode that reads from a memory buffer of unknown size. */ -abstract class BufferReadOpcode extends BufferAccessOpcode { +abstract class UnsizedBufferReadOpcode extends UnsizedBufferAccessOpcode { final override MemoryAccessKind getReadMemoryAccess() { result instanceof BufferMemoryAccess } } @@ -261,9 +267,7 @@ abstract class EntireAllocationReadOpcode extends EntireAllocationAccessOpcode { /** * An opcode that accesses a memory buffer whose size is determined by a `BufferSizeOperand`. */ -abstract class SizedBufferAccessOpcode extends Opcode { - final override predicate hasAddressOperand() { any() } - +abstract class SizedBufferAccessOpcode extends BufferAccessOpcode { final override predicate hasBufferSizeOperand() { any() } } @@ -666,16 +670,16 @@ module Opcode { final override string toString() { result = "IndirectMayWriteSideEffect" } } - class BufferReadSideEffect extends ReadSideEffectOpcode, BufferReadOpcode, TBufferReadSideEffect { + class BufferReadSideEffect extends ReadSideEffectOpcode, UnsizedBufferReadOpcode, TBufferReadSideEffect { final override string toString() { result = "BufferReadSideEffect" } } - class BufferMustWriteSideEffect extends WriteSideEffectOpcode, BufferWriteOpcode, + class BufferMustWriteSideEffect extends WriteSideEffectOpcode, UnsizedBufferWriteOpcode, TBufferMustWriteSideEffect { final override string toString() { result = "BufferMustWriteSideEffect" } } - class BufferMayWriteSideEffect extends WriteSideEffectOpcode, BufferWriteOpcode, MayWriteOpcode, + class BufferMayWriteSideEffect extends WriteSideEffectOpcode, UnsizedBufferWriteOpcode, MayWriteOpcode, TBufferMayWriteSideEffect { final override string toString() { result = "BufferMayWriteSideEffect" } } diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll index c65570c5999..2dfd3f60902 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll @@ -26,7 +26,7 @@ private predicate hasResultMemoryAccess( type = languageType.getIRType() and isIndirectOrBufferMemoryAccess(instr.getResultMemoryAccess()) and (if instr.hasResultMayMemoryAccess() then isMayAccess = true else isMayAccess = false) and - if type.getByteSize() > 0 + if exists(type.getByteSize()) then endBitOffset = Ints::add(startBitOffset, Ints::mul(type.getByteSize(), 8)) else endBitOffset = Ints::unknown() ) @@ -43,7 +43,7 @@ private predicate hasOperandMemoryAccess( type = languageType.getIRType() and isIndirectOrBufferMemoryAccess(operand.getMemoryAccess()) and (if operand.hasMayReadMemoryAccess() then isMayAccess = true else isMayAccess = false) and - if type.getByteSize() > 0 + if exists(type.getByteSize()) then endBitOffset = Ints::add(startBitOffset, Ints::mul(type.getByteSize(), 8)) else endBitOffset = Ints::unknown() ) diff --git a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll index 93e5c43bf3b..25b1677958d 100644 --- a/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll +++ b/cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll @@ -487,7 +487,7 @@ class TranslatedSideEffect extends TranslatedElement, TTranslatedArgumentSideEff } override CppType getInstructionOperandType(InstructionTag tag, TypedOperandTag operandTag) { - if hasSpecificReadSideEffect(any(Opcode::BufferReadSideEffect op)) + if hasSpecificReadSideEffect(any(BufferAccessOpcode op)) then result = getUnknownType() and tag instanceof OnlyInstructionTag and diff --git a/csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll b/csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll index 886183c32ba..3fb716d78e0 100644 --- a/csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll +++ b/csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll @@ -213,23 +213,29 @@ abstract class IndirectReadOpcode extends IndirectMemoryAccessOpcode { } /** - * An opcode that accesses a memory buffer of unknown size. + * An opcode that accesses a memory buffer. */ abstract class BufferAccessOpcode extends Opcode { final override predicate hasAddressOperand() { any() } } +/** + * An opcode that accesses a memory buffer of unknown size. + */ +abstract class UnsizedBufferAccessOpcode extends BufferAccessOpcode { +} + /** * An opcode that writes to a memory buffer of unknown size. */ -abstract class BufferWriteOpcode extends BufferAccessOpcode { +abstract class UnsizedBufferWriteOpcode extends UnsizedBufferAccessOpcode { final override MemoryAccessKind getWriteMemoryAccess() { result instanceof BufferMemoryAccess } } /** * An opcode that reads from a memory buffer of unknown size. */ -abstract class BufferReadOpcode extends BufferAccessOpcode { +abstract class UnsizedBufferReadOpcode extends UnsizedBufferAccessOpcode { final override MemoryAccessKind getReadMemoryAccess() { result instanceof BufferMemoryAccess } } @@ -261,9 +267,7 @@ abstract class EntireAllocationReadOpcode extends EntireAllocationAccessOpcode { /** * An opcode that accesses a memory buffer whose size is determined by a `BufferSizeOperand`. */ -abstract class SizedBufferAccessOpcode extends Opcode { - final override predicate hasAddressOperand() { any() } - +abstract class SizedBufferAccessOpcode extends BufferAccessOpcode { final override predicate hasBufferSizeOperand() { any() } } @@ -666,16 +670,16 @@ module Opcode { final override string toString() { result = "IndirectMayWriteSideEffect" } } - class BufferReadSideEffect extends ReadSideEffectOpcode, BufferReadOpcode, TBufferReadSideEffect { + class BufferReadSideEffect extends ReadSideEffectOpcode, UnsizedBufferReadOpcode, TBufferReadSideEffect { final override string toString() { result = "BufferReadSideEffect" } } - class BufferMustWriteSideEffect extends WriteSideEffectOpcode, BufferWriteOpcode, + class BufferMustWriteSideEffect extends WriteSideEffectOpcode, UnsizedBufferWriteOpcode, TBufferMustWriteSideEffect { final override string toString() { result = "BufferMustWriteSideEffect" } } - class BufferMayWriteSideEffect extends WriteSideEffectOpcode, BufferWriteOpcode, MayWriteOpcode, + class BufferMayWriteSideEffect extends WriteSideEffectOpcode, UnsizedBufferWriteOpcode, MayWriteOpcode, TBufferMayWriteSideEffect { final override string toString() { result = "BufferMayWriteSideEffect" } }