Skip to content
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

Disallow using arrays with NewHolder #12514

Open
omajid opened this issue Apr 17, 2019 · 1 comment
Open

Disallow using arrays with NewHolder #12514

omajid opened this issue Apr 17, 2019 · 1 comment

Comments

@omajid
Copy link
Member

omajid commented Apr 17, 2019

I have recently found and fixed some uses where NewHolder was being used with arrays instead of NewArrayHolder.

I wonder if it would be possible to modify the NewHolder code so that it fails to compile examples like this:

NewHolder<BYTE> code(new BYTE[size]);  // Should fail
NewHolder<BYTE> pNewSig = new BYTE[size];   // Should fail

The static_asserts don't seem to be working for BYTE and other non-char array types.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@omajid
Copy link
Member Author

omajid commented May 10, 2021

Just for fun, I tried out this change:

diff --git a/src/coreclr/inc/holder.h b/src/coreclr/inc/holder.h
index 20f6aeb9646..41d0d6f6389 100644
--- a/src/coreclr/inc/holder.h
+++ b/src/coreclr/inc/holder.h
@@ -979,6 +979,8 @@ FORCEINLINE void Delete(TYPE *value)
                   "Must use NewArrayHolder (not NewHolder) for strings.");
     static_assert(!std::is_same<typename std::remove_cv<TYPE>::type, CHAR>::value,
                   "Must use NewArrayHolder (not NewHolder) for strings.");
+    static_assert(!std::is_same<typename std::remove_cv<TYPE>::type, BYTE>::value,
+                  "Must use NewArrayHolder (not NewHolder) for byte arrays.");
 
     delete value;
 }

It only flagged one more issue:

diff --git a/src/coreclr/vm/objectlist.cpp b/src/coreclr/vm/objectlist.cpp
index ac80ca33bac..34a9d7d097c 100644
--- a/src/coreclr/vm/objectlist.cpp
+++ b/src/coreclr/vm/objectlist.cpp
@@ -179,7 +179,7 @@ UnsynchronizedBlockAllocator::Allocate( size_t size )
 
     _ASSERTE( size <= this->blockSize_ );
 
-    NewHolder<BYTE> buffer;
+    NewArrayHolder<BYTE> buffer;
 
     S_SIZE_T sizecheck = S_SIZE_T(this->offset_) + S_SIZE_T(size) ;
     if( sizecheck.IsOverflow() )

But it's really a false positive, since this code calls buffer.SuppressRelease() in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants