vma/dev: Fix new/delete size mismatch. #1149

strike-kokhotnikov · 2025-11-13T13:35:36Z

Description

Trivial child class was created with new operator, but delete operator were applied to base class instance. Changing the classes to non-trivial is impossible, because they used in external rdma-core library.

What

Fix mismatch in size of new and delete structure.

Why ?

Fixed memory leak problem.

Change type

What kind of change does this PR introduce?

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)

greptile-apps · 2025-11-13T13:38:45Z

Greptile Overview

Greptile Summary

Fixed new/delete size mismatch by implementing custom allocation strategy using aligned_alloc + placement new.

Key Changes

Replaced standard new operator with new_malloc template helper for flow data structures
Changed destructor to use free() instead of delete to properly deallocate memory from aligned_alloc
Added static assertion to ensure types are trivially destructible (required for aligned_alloc + placement new pattern)
Exception handling via std::bad_alloc when allocation fails (caught by existing try/catch blocks in ring_slave.cpp)

Rationale

The structures (attach_flow_data_*_t) are used by external rdma-core library and must remain trivial (no virtual destructors). Previously, child class instances were allocated with new but deallocated through base class pointer with delete, causing size mismatch. The new approach uses aligned_alloc (C standard library) which returns the exact allocation size to free(), avoiding the mismatch while maintaining trivial class requirements.

Confidence Score: 4/5

This PR is safe to merge with minor exception handling consideration
The PR correctly addresses the new/delete size mismatch by using aligned_alloc + placement new pattern. The implementation includes proper static assertions for trivially destructible types and throws std::bad_alloc on allocation failure (which is caught in ring_slave.cpp). The previous comments correctly identified that the new implementation throws exceptions rather than returning NULL, which changes error handling semantics - however this is properly caught by existing exception handlers. Score is 4 rather than 5 because the exception-throwing behavior differs from typical allocation patterns in this codebase (which often use std::nothrow), though it's handled correctly.
src/vma/dev/rfs.h - review the new_malloc implementation for exception handling semantics

Important Files Changed

File Analysis

Filename	Score	Overview
src/vma/dev/rfs.h	4/5	Adds `new_malloc` template helper to allocate memory with `aligned_alloc` and placement `new`, replacing standard `new` operator; includes proper exception handling via `std::bad_alloc` when allocation fails
src/vma/dev/rfs.cpp	5/5	Changes destructor to use `free()` instead of `delete` operator to match memory allocated via `aligned_alloc` in the new `new_malloc` helper
src/vma/dev/rfs_mc.cpp	5/5	Replaces `new` operator calls with `new_malloc` template function for all flow data allocations (3 locations)
src/vma/dev/rfs_uc.cpp	5/5	Replaces `new` operator calls with `new_malloc` template function for all flow data allocations (3 locations)

Sequence Diagram

sequenceDiagram
    participant UC as rfs_uc/rfs_mc
    participant RFS as rfs (base)
    participant NM as new_malloc<T>
    participant AA as aligned_alloc
    participant PN as placement new
    participant D as ~rfs destructor

    Note over UC,D: Allocation Flow
    UC->>NM: new_malloc<attach_flow_data_*_t>(qp_mgr)
    NM->>NM: static_assert(is_trivially_destructible)
    NM->>AA: aligned_alloc(alignof(T), sizeof(T))
    alt allocation fails
        AA-->>NM: NULL
        NM->>NM: throw std::bad_alloc
    else allocation succeeds
        AA-->>NM: aligned memory pointer
        NM->>PN: new(p) T(args...)
        PN-->>NM: constructed object
        NM-->>UC: return T*
    end
    UC->>RFS: add to m_attach_flow_data_vector

    Note over UC,D: Deallocation Flow
    D->>D: iterate m_attach_flow_data_vector
    D->>D: free(pointer)
    Note over D: Uses free() not delete<br/>to match aligned_alloc

greptile-apps

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-13T13:38:44Z

src/vma/dev/rfs.h

+	template <class T, typename ...Args>
+	T * new_malloc(Args ... args) {
+		static_assert(std::is_trivially_destructible<T>::value == true);
+		void * p = aligned_alloc(alignof(T), sizeof(T));


logic: aligned_alloc returns NULL on failure, but there's no null check before placement new

Suggested change

void * p = aligned_alloc(alignof(T), sizeof(T));

void * p = aligned_alloc(alignof(T), sizeof(T));

if (!p) return NULL;

return new(p) T(args...);

svc-nbu-swx-media · 2025-11-13T13:40:08Z

Can one of the admins verify this patch?

greptile-apps

_{4 files reviewed, 6 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-13T13:46:57Z

src/vma/dev/rfs_uc.cpp

+				attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v1_t>(p_ring->m_p_qp_mgr);
 				ibv_flow_spec_ib_set_by_dst_qpn(&(attach_flow_data_ib_v1->ibv_flow_attr.ib),


logic: missing null check after allocation - if new_malloc fails and returns null, dereferencing on line 63 causes crash

Suggested change

attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v1_t>(p_ring->m_p_qp_mgr);

ibv_flow_spec_ib_set_by_dst_qpn(&(attach_flow_data_ib_v1->ibv_flow_attr.ib),

attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v1_t>(p_ring->m_p_qp_mgr);

if (!attach_flow_data_ib_v1) return false;

ibv_flow_spec_ib_set_by_dst_qpn(&(attach_flow_data_ib_v1->ibv_flow_attr.ib),

greptile-apps · 2025-11-13T13:46:58Z

src/vma/dev/rfs_uc.cpp

+			attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v2_t>(p_ring->m_p_qp_mgr);

 			p_ipv4 = &(attach_flow_data_ib_v2->ibv_flow_attr.ipv4);


logic: missing null check after allocation - if new_malloc fails and returns null, dereferencing on line 73 causes crash

Suggested change

attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v2_t>(p_ring->m_p_qp_mgr);

p_ipv4 = &(attach_flow_data_ib_v2->ibv_flow_attr.ipv4);

attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v2_t>(p_ring->m_p_qp_mgr);

if (!attach_flow_data_ib_v2) return false;

p_ipv4 = &(attach_flow_data_ib_v2->ibv_flow_attr.ipv4);

greptile-apps · 2025-11-13T13:46:59Z

src/vma/dev/rfs_uc.cpp

+			attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

 			ibv_flow_spec_eth_set(&(attach_flow_data_eth->ibv_flow_attr.eth),


logic: missing null check after allocation - if new_malloc fails and returns null, dereferencing on line 82 causes crash

Suggested change

attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

ibv_flow_spec_eth_set(&(attach_flow_data_eth->ibv_flow_attr.eth),

attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

if (!attach_flow_data_eth) return false;

ibv_flow_spec_eth_set(&(attach_flow_data_eth->ibv_flow_attr.eth),

greptile-apps · 2025-11-13T13:46:59Z

src/vma/dev/rfs_mc.cpp

+				attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_v1_t>(p_ring->m_p_qp_mgr);

 				uint8_t dst_gid[16];


logic: missing null check after allocation - if new_malloc fails and returns null, dereferencing on line 61 causes crash

Suggested change

attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_v1_t>(p_ring->m_p_qp_mgr);

uint8_t dst_gid[16];

attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_v1_t>(p_ring->m_p_qp_mgr);

if (!attach_flow_data_ib_v1) return false;

uint8_t dst_gid[16];

greptile-apps · 2025-11-13T13:47:00Z

src/vma/dev/rfs_mc.cpp

+			attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_v2_t>(p_ring->m_p_qp_mgr);

 			ibv_flow_spec_ipv4_set(&(attach_flow_data_ib_v2->ibv_flow_attr.ipv4),


logic: missing null check after allocation - if new_malloc fails and returns null, dereferencing on line 75 causes crash

Suggested change

attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_v2_t>(p_ring->m_p_qp_mgr);

ibv_flow_spec_ipv4_set(&(attach_flow_data_ib_v2->ibv_flow_attr.ipv4),

attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_v2_t>(p_ring->m_p_qp_mgr);

if (!attach_flow_data_ib_v2) return false;

ibv_flow_spec_ipv4_set(&(attach_flow_data_ib_v2->ibv_flow_attr.ipv4),

greptile-apps · 2025-11-13T13:47:01Z

src/vma/dev/rfs_mc.cpp

+			attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

 			uint8_t dst_mac[6];


logic: missing null check after allocation - if new_malloc fails and returns null, dereferencing on line 93 causes crash

Suggested change

attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

uint8_t dst_mac[6];

attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

if (!attach_flow_data_eth) return false;

uint8_t dst_mac[6];

greptile-apps

_{4 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps bot reviewed Nov 13, 2025

View reviewed changes

strike-kokhotnikov force-pushed the fix/kokhotnikov/new-delete-size-mistmatch branch from 5ec33b3 to b04c1ed Compare November 13, 2025 13:43

greptile-apps bot reviewed Nov 13, 2025

View reviewed changes

vma/dev: Fix new/delete size mismatch.

ec5e44a

strike-kokhotnikov force-pushed the fix/kokhotnikov/new-delete-size-mistmatch branch from b04c1ed to ec5e44a Compare November 13, 2025 14:32

greptile-apps bot reviewed Nov 13, 2025

View reviewed changes

strike-kokhotnikov mentioned this pull request Nov 17, 2025

Use-after-free and heap-size-mismatch fix. #1136

Closed

13 tasks

		attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v1_t>(p_ring->m_p_qp_mgr);
		ibv_flow_spec_ib_set_by_dst_qpn(&(attach_flow_data_ib_v1->ibv_flow_attr.ib),

		attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_ipv4_tcp_udp_v2_t>(p_ring->m_p_qp_mgr);

		p_ipv4 = &(attach_flow_data_ib_v2->ibv_flow_attr.ipv4);

		attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

		ibv_flow_spec_eth_set(&(attach_flow_data_eth->ibv_flow_attr.eth),

		attach_flow_data_ib_v1 = new_malloc<attach_flow_data_ib_v1_t>(p_ring->m_p_qp_mgr);

		uint8_t dst_gid[16];

		attach_flow_data_ib_v2 = new_malloc<attach_flow_data_ib_v2_t>(p_ring->m_p_qp_mgr);

		ibv_flow_spec_ipv4_set(&(attach_flow_data_ib_v2->ibv_flow_attr.ipv4),

		attach_flow_data_eth = new_malloc<attach_flow_data_eth_ipv4_tcp_udp_t>(p_ring->m_p_qp_mgr);

		uint8_t dst_mac[6];

vma/dev: Fix new/delete size mismatch. #1149

Are you sure you want to change the base?

vma/dev: Fix new/delete size mismatch. #1149

Uh oh!

Conversation

strike-kokhotnikov commented Nov 13, 2025

Description

What

Why ?

Change type

Check list

Uh oh!

greptile-apps bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Key Changes

Rationale

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

svc-nbu-swx-media commented Nov 13, 2025

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greptile-apps bot commented Nov 13, 2025 •

edited

Loading