Skip to content

Conversation

@strike-kokhotnikov
Copy link

@strike-kokhotnikov strike-kokhotnikov commented Nov 13, 2025

Description

The changes is required to run the code compiled with ASAN. The problem is that both ASAN and libvma override libc standard functions and during initialization process some kind of "racing conditions" is possible. Function read_file_to_int most probably, has false-positive alert, but function env_to_cpuset is not. The default value -1 is passed to from env_to_cpuset to list_to_cpuset function, which can't parse it correctly.

What

Fix ASAN problems.

Why ?

ASAN can help to debug memory problems in some cases much more efficient, than e.g. valgrind.

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

@svc-nbu-swx-media
Copy link

Can one of the admins verify this patch?

@greptile-apps
Copy link

greptile-apps bot commented Nov 13, 2025

Greptile Overview

Greptile Summary

This PR fixes ASAN compatibility issues that occur when both ASAN and libvma override libc functions during initialization.

Key Changes:

  • Replaced C++ std::ifstream with C-style open/read/close in read_file_to_int to avoid potential race conditions with ASAN's function interception
  • Added explicit handling for the "-1" default value in env_to_cpuset before attempting to parse it as hex or cpulist, preventing parsing errors in list_to_cpuset

Both changes are necessary to enable ASAN-based debugging and memory analysis for VMA.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - changes are focused and address specific ASAN compatibility issues
  • Score reflects well-targeted fixes for ASAN compatibility. The utils.cpp change correctly handles file operations with proper resource cleanup. The sys_vars.cpp change properly handles the "-1" special case that would otherwise fail parsing. Minor point deduction due to the previous comment about a potential fd leak that was actually already fixed in the code.
  • No files require special attention - both changes are straightforward bug fixes

Important Files Changed

File Analysis

Filename Score Overview
src/vma/util/utils.cpp 4/5 Replaced C++ ifstream with C-style open/read/close to avoid ASAN conflicts during initialization
src/vma/util/sys_vars.cpp 5/5 Added special handling for "-1" default value to prevent parsing errors in list_to_cpuset

Sequence Diagram

sequenceDiagram
    participant Init as VMA Initialization
    participant SysVars as sys_vars.cpp
    participant Utils as utils.cpp
    participant LibC as libc (open/read)
    
    Note over Init,LibC: ASAN-safe initialization
    
    Init->>Utils: read_file_to_int(path, default)
    Utils->>LibC: open(path, O_RDONLY)
    LibC-->>Utils: fd
    alt fd >= 0
        Utils->>LibC: read(fd, buffer, 20)
        LibC-->>Utils: bytes read
        alt sscanf succeeds
            Utils->>LibC: close(fd)
            Utils-->>Init: parsed value
        else sscanf fails
            Utils->>LibC: close(fd)
            Utils-->>Init: default_value
        end
    else fd < 0
        Utils-->>Init: default_value
    end
    
    Init->>SysVars: env_to_cpuset("-1", cpu_set)
    alt input is "-1"
        SysVars->>SysVars: CPU_ZERO(cpu_set)
        SysVars-->>Init: success (0)
    else input is hex (0x...)
        SysVars->>SysVars: hex_to_cpuset()
        SysVars-->>Init: result
    else input is list
        SysVars->>SysVars: list_to_cpuset()
        SysVars-->>Init: result
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

{
int ret;
char* start = strdup(orig_start); // save the caller string from strtok destruction.
memset(&(cpu_set->__bits), 0, sizeof(cpu_set->__bits));
Copy link

Choose a reason for hiding this comment

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

style: directly accessing __bits field breaks encapsulation - use CPU_ZERO(cpu_set) instead

Suggested change
memset(&(cpu_set->__bits), 0, sizeof(cpu_set->__bits));
CPU_ZERO(cpu_set);

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

if (sz > 0) {
int value;
c[sz] = '\0';
int n = sscanf(reinterpret_cast<const char *>(&c), "%d", &value);
Copy link

Choose a reason for hiding this comment

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

style: unnecessary cast - c already decays to const char*

Suggested change
int n = sscanf(reinterpret_cast<const char *>(&c), "%d", &value);
int n = sscanf(c, "%d", &value);

{
int ret;
char* start = strdup(orig_start); // save the caller string from strtok destruction.
CPU_ZERO(cpu_set);
Copy link

Choose a reason for hiding this comment

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

style: CPU_ZERO will be called again in hex_to_cpuset or list_to_cpuset - move this call to after the "-1" check to avoid redundant operation

Suggested change
CPU_ZERO(cpu_set);
int len = strlen(orig_start);
if (len == 2 && orig_start[0] == '-' && orig_start[1] == '1') {
CPU_ZERO(cpu_set);
return 0;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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