Skip to content

Fix potential out-of-bounds access by checking if start is empty before removing prefix #681

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.

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

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

flylai
Copy link
Contributor

@flylai flylai commented Jan 5, 2025

If the code contains compiler builtin functions, it will crash with /usr/include/c++/14.2.1/string_view:297: constexpr void std::basic_string_view<_CharT, _Traits>::remove_prefix(size_type) [with _CharT = char; _Traits = std::char_traits<char>; size_type = long unsigned int]: Assertion 'this->_M_len >= __n' failed.

In other words: "support compiler builtin functions"

eg:

#include <inttypes.h>

/* WARNING: undefined if a = 0 */
static inline int clz32(unsigned int a)
{
    return __builtin_clz(a);
}

/* WARNING: undefined if a = 0 */
static inline int clz64(uint64_t a)
{
    return __builtin_clzll(a);
}

/* WARNING: undefined if a = 0 */
static inline int ctz32(unsigned int a)
{
    return __builtin_ctz(a);
}

/* WARNING: undefined if a = 0 */
static inline int ctz64(uint64_t a)
{
    return __builtin_ctzll(a);
}

after fixes, the result is:

#include <inttypes.h>

static inline int clz32(unsigned int a)
{
  return __builtin_clz(a);
}

 __attribute__((nothrow)) __attribute__((const)) extern int __builtin_clz(unsigned int);

static inline int clz64(uint64_t a)
{
  return __builtin_clzll(static_cast<unsigned long long>(a));
}

 __attribute__((nothrow)) __attribute__((const)) extern int __builtin_clzll(unsigned long long);

static inline int ctz32(unsigned int a)
{
  return __builtin_ctz(a);
}

 __attribute__((nothrow)) __attribute__((const)) extern int __builtin_ctz(unsigned int);

static inline int ctz64(uint64_t a)
{
  return __builtin_ctzll(static_cast<unsigned long long>(a));
}

 __attribute__((nothrow)) __attribute__((const)) extern int __builtin_ctzll(unsigned long long);

@andreasfertig
Copy link
Owner

Hello @flylai,

thanks for the PR!

Could you please also add a test for this change? You find them under tests. Since this is also a fix you can use the notation Issue681.cpp and Issue681.expect. Copy your initially failing code into the .cpp and the working output of C++ Insights into .expect. Thank you!

Andreas

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (a84a979) to head (8d29d87).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #681   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files          20       20           
  Lines        6534     6535    +1     
=======================================
+ Hits         6394     6395    +1     
  Misses        140      140           
Flag Coverage Δ
insights-macos 97.13% <100.00%> (+<0.01%) ⬆️
insights-ubuntu-amd64-libcxx-No 98.10% <100.00%> (+<0.01%) ⬆️
insights-ubuntu-amd64-libcxx-Yes 98.08% <100.00%> (+<0.01%) ⬆️
insights-ubuntu-arm64-libcxx-No 98.06% <100.00%> (+<0.01%) ⬆️
insights-ubuntu-arm64-libcxx-Yes 98.04% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@flylai flylai force-pushed the main branch 2 times, most recently from af89105 to 59bd291 Compare January 6, 2025 12:06
@flylai
Copy link
Contributor Author

flylai commented Jan 6, 2025

Hi @andreasfertig
Test case have been added to commit.

@andreasfertig
Copy link
Owner

Hello @flylai,

thanks for the test! Unfortunately, it failed. I suspect for the output you posted you had other C++ Insights options enabled?
While you can teach the test this, I think its best to had the plain transformation tested. Could you please adjust the test result?

Andreas

@flylai
Copy link
Contributor Author

flylai commented Jan 6, 2025

Hi @andreasfertig
I am using Archlinux with clang 18.1.8, and I retry the tests with build arguments as follows

cd cppinsights
mkdir build
cd build
cmake .. -DINSIGHTS_USE_SYSTEM_INCLUDES=off -DCLANG_LINK_CLANG_DYLIB=on -DLLVM_LINK_LLVM_DYLIB=on -DCMAKE_CXX_COMPILER=clang++
make tests -j24

and the cmake output:

-- The CXX compiler identification is Clang 18.1.8
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/clang++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- llvm_config(LLVM_CXXFLAGS)=>-I/usr/include    -fno-exceptions -funwind-tables -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS;
-- llvm_config(LLVM_LDFLAGS)=>-L/usr/lib;
-- llvm_config(LLVM_LIBS2)=>-lLLVM-18;
-- llvm_config(LLVM_LIBDIR)=>/usr/lib;
-- llvm_config(LLVM_INCLUDE_DIR)=>/usr/include;
-- llvm_config(LLVM_SYSTEM_LIBS2)=>;
-- llvm_config(LLVM_PACKAGE_VERSION)=>18.1.8;
-- Generating version.h
-- Found clang-tidy: /usr/bin/clang-tidy
-- Could not find the program include-what-you-use
-- Found Doxygen: /usr/bin/doxygen (found version "1.13.1") found components: doxygen dot
-- Found Python3: /home/lkm/pyvenv/bin/python3.13 (found version "3.13.1") found components: Interpreter
-- Found Python3: /home/lkm/pyvenv/bin/python3.13. Target tests enabled.
-- 
-- [ Build summary ]
-- BUILD_INSIGHTS_OUTSIDE_LLVM : On
-- CMAKE_GENERATOR       : Unix Makefiles
-- CMAKE_EXE_LINKER_FLAGS: 
-- CMAKE_LINKER          : /usr/bin/ld
-- CMAKE_OSX_ARCHITECTURES : 
-- Compiler ID           : Clang
-- Compiler version      : 18.1.8
-- Compiler path         : /usr/bin/clang++
-- llvm-config           : /usr/bin/llvm-config
-- Min LLVM major version: 18
-- Install path          : /usr/local
-- Clang resource dir    : 
-- CMAKE_SOURCE_DIR      : /home/lkm/dev/cppinsights
-- CMAKE_BINARY_DIR      : /home/lkm/dev/cppinsights/build
-- Git repo url          : https://github.com/andreasfertig/cppinsights.git
-- Git commit hash       : 59bd291cf159e28ccbb955c977edb72dc18fd02b
-- Debug                 : OFF
-- Code Coverage         : OFF
-- Static linking        : OFF
-- Strip executable      : ON
-- Elevate includes:     : off
-- clang-tidy            : Off
-- include-what-you-use  : Off
-- 
-- Configuring done (0.3s)
-- Generating done (0.0s)
-- Build files have been written to: /home/lkm/dev/cppinsights/build

and the latest output is

#include <inttypes.h>

static inline int clz32(unsigned int a)
{
  return __builtin_clz(a);
}

static inline int clz64(uint64_t a)
{
  return __builtin_clzll(static_cast<unsigned long long>(a));
}

static inline int ctz32(unsigned int a)
{
  return __builtin_ctz(a);
}

static inline int ctz64(uint64_t a)
{
  return __builtin_ctzll(static_cast<unsigned long long>(a));
}

It is still different with the macOS test result, I will adjust the test result.

@andreasfertig andreasfertig merged commit d1674a3 into andreasfertig:main Jan 6, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants