Subtle but Deadly ...
-
A few years ago, I wrote a function that dealt with creating an array of integers. There were some functional requirements that prohibited use of STL, so the function had to be written using primitive types. The following is basically what it looked like:
int* CopySetAndDoSomething(int* ia, int size) { int* pRet = new int[size]; memcpy(pRet, ia, size); for (int i = 0; i < size; ++i) { pRet[i] += 5; } return pRet; }Now, the requirements specified that ia would always be non-null and size would always be >0. However, the method was then used by someone else in an MFC application where a vector was passed in with a size of 0. This yielded interesting results in Windows.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
This line memcpy(pRet, ia, size); may also be a little unsafe. If size bigger than ia's length - then it may try and access un-readable memory (may not be a common scenario) and then you get an access violation.
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blog -
This line memcpy(pRet, ia, size); may also be a little unsafe. If size bigger than ia's length - then it may try and access un-readable memory (may not be a common scenario) and then you get an access violation.
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blogNishant Sivakumar wrote:
may also be a little unsafe. If size bigger than ia's length - then it may try and access un-readable memory (may not be a common scenario) and then you get an access violation
Yes, and he would have to add a check for that in the code.
-
How come ia isn't checked to be zero length? I would test for that condition and return a value accordingly.
That was the fix ... but the precondition stated that 0 would never be entered, so it wasn't checked originally.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
This line memcpy(pRet, ia, size); may also be a little unsafe. If size bigger than ia's length - then it may try and access un-readable memory (may not be a common scenario) and then you get an access violation.
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blogWhile that is certainly possible, it is hard to verify that the memory you are looking at is actually the size you specified. Many of the old C-style API methods work like this and expect you to pass in the correct size. If I could have used STL, it would have been much simpler and avoided both of those issues.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
A few years ago, I wrote a function that dealt with creating an array of integers. There were some functional requirements that prohibited use of STL, so the function had to be written using primitive types. The following is basically what it looked like:
int* CopySetAndDoSomething(int* ia, int size) { int* pRet = new int[size]; memcpy(pRet, ia, size); for (int i = 0; i < size; ++i) { pRet[i] += 5; } return pRet; }Now, the requirements specified that ia would always be non-null and size would always be >0. However, the method was then used by someone else in an MFC application where a vector was passed in with a size of 0. This yielded interesting results in Windows.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
Zac Howland wrote:
int* pRet = new int[size]; memcpy(pRet, ia, size);
Hmmm - strikes me there's a bug here as well (assuming
sizeof(int)!= 1) ? First line allocates size ints (i.esize*sizeof(int)bytes, second one copiessizebytes. Should I just presume a missing '* sizeof(int)' in thememcmpline? :-) -
Zac Howland wrote:
int* pRet = new int[size]; memcpy(pRet, ia, size);
Hmmm - strikes me there's a bug here as well (assuming
sizeof(int)!= 1) ? First line allocates size ints (i.esize*sizeof(int)bytes, second one copiessizebytes. Should I just presume a missing '* sizeof(int)' in thememcmpline? :-)Stuart Dootson wrote:
Should I just presume a missing '* sizeof(int)' in the memcmp line?
Actually, that is just a typo on my part. The correct size was passed (size * sizeof(int)).
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
A few years ago, I wrote a function that dealt with creating an array of integers. There were some functional requirements that prohibited use of STL, so the function had to be written using primitive types. The following is basically what it looked like:
int* CopySetAndDoSomething(int* ia, int size) { int* pRet = new int[size]; memcpy(pRet, ia, size); for (int i = 0; i < size; ++i) { pRet[i] += 5; } return pRet; }Now, the requirements specified that ia would always be non-null and size would always be >0. However, the method was then used by someone else in an MFC application where a vector was passed in with a size of 0. This yielded interesting results in Windows.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
This line memcpy(pRet, ia, size); may also be a little unsafe. If size bigger than ia's length - then it may try and access un-readable memory (may not be a common scenario) and then you get an access violation.
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
Currently working on C++/CLI in Action for Manning Publications. Also visit the Ultimate Toolbox blogNishant Sivakumar wrote:
may also be a little unsafe.
unsafe as far as security? Buffer overflow attacks requires the "target" buffer to be smaller than the memcpy.size argument which is not the case here.
Nishant Sivakumar wrote:
then it may try and access un-readable memory (may not be a common scenario) and then you get an access violation.
Even if you don't get an access violation (not very common on a read), you will get garbage data.
led mike
-
Zac Howland wrote:
memcpy(pRet, ia, size);
shouldn't that be
memcpy(pRet, ia, sizeof(int)*size);led mike
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
That was the fix ... but the precondition stated that 0 would never be entered, so it wasn't checked originally.
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
Who called the function? You should've slapped them on their fingers for not living up to their promises. ;)
-- A Stern Warning of Things to Come
-
Who called the function? You should've slapped them on their fingers for not living up to their promises. ;)
-- A Stern Warning of Things to Come
Jörgen Sigvardsson wrote:
Who called the function? You should've slapped them on their fingers for not living up to their promises
My boss. So, obviously the problem was in my code ... and not in the requirements that he wrote up. That said, I should have written it better to avoid this kind of behavior anyway, but you have to learn somehow right?
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac
-
Jörgen Sigvardsson wrote:
Who called the function? You should've slapped them on their fingers for not living up to their promises
My boss. So, obviously the problem was in my code ... and not in the requirements that he wrote up. That said, I should have written it better to avoid this kind of behavior anyway, but you have to learn somehow right?
If you decide to become a software engineer, you are signing up to have a 1/2" piece of silicon tell you exactly how stupid you really are for 8 hours a day, 5 days a week Zac