Double Bug
-
That is the first bug (memory leak):
int main() { Stuff myStuff; char* pTemp = getName(); myStuff.setName(pTemp); delete pTemp; pTemp = getAddress(); myStuff.setAddress(pTemp); delete pTemp; // do a whole lot of really cool things here }Which should reveal the second bug ...
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 delete the char arrays right away like that, your Stuff object will have dangling pointers, and you'll have a lot of fun doing those really cool things afterwards :-)
-
If you delete the char arrays right away like that, your Stuff object will have dangling pointers, and you'll have a lot of fun doing those really cool things afterwards :-)
Exactly! The fix for both bugs looked like this:
// class definition class Stuff { public: Stuff() {} ~Stuff() {} void setName(const char* name) {_name = name;} void setAddress(const char* addr) {_address = addr;} private: std::string _name; std::string _address; }; // usage // the following functions were actually part of a 3rd-party library, so they could not be changed char* getName() { char* pRet = new char[20]; memset(pRet, 0, 20); strcpy(pRet, "Charlie"); return pRet; } char* getAddress() { char* pRet = new char[20]; memset(pRet, 0, 20); strcpy(pRet, "United States"); return pRet; } int main() { Stuff myStuff; char* pTemp = getName(); myStuff.setName(pTemp); delete [] pTemp; pTemp = getAddress(); myStuff.setAddress(pTemp); delete [] pTemp; // do some really cool stuff here }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
-
Exactly! The fix for both bugs looked like this:
// class definition class Stuff { public: Stuff() {} ~Stuff() {} void setName(const char* name) {_name = name;} void setAddress(const char* addr) {_address = addr;} private: std::string _name; std::string _address; }; // usage // the following functions were actually part of a 3rd-party library, so they could not be changed char* getName() { char* pRet = new char[20]; memset(pRet, 0, 20); strcpy(pRet, "Charlie"); return pRet; } char* getAddress() { char* pRet = new char[20]; memset(pRet, 0, 20); strcpy(pRet, "United States"); return pRet; } int main() { Stuff myStuff; char* pTemp = getName(); myStuff.setName(pTemp); delete [] pTemp; pTemp = getAddress(); myStuff.setAddress(pTemp); delete [] pTemp; // do some really cool stuff here }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 Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.
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 -
Exactly! The fix for both bugs looked like this:
// class definition class Stuff { public: Stuff() {} ~Stuff() {} void setName(const char* name) {_name = name;} void setAddress(const char* addr) {_address = addr;} private: std::string _name; std::string _address; }; // usage // the following functions were actually part of a 3rd-party library, so they could not be changed char* getName() { char* pRet = new char[20]; memset(pRet, 0, 20); strcpy(pRet, "Charlie"); return pRet; } char* getAddress() { char* pRet = new char[20]; memset(pRet, 0, 20); strcpy(pRet, "United States"); return pRet; } int main() { Stuff myStuff; char* pTemp = getName(); myStuff.setName(pTemp); delete [] pTemp; pTemp = getAddress(); myStuff.setAddress(pTemp); delete [] pTemp; // do some really cool stuff here }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:
The fix for both bugs looked like this [...]
Thats not a fix, that is proper programming technique! :-D
"We trained hard, but it seemed that every time we were beginning to form up into teams we would be reorganised. I was to learn later in life that we tend to meet any new situation by reorganising: and a wonderful method it can be for creating the illusion of progress, while producing confusion, inefficiency and demoralisation." -- Caius Petronius, Roman Consul, 66 A.D.
-
Zac Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.
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:
Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.
The leak was not so subtle. However, the memory violation was more subtle in the original code because the
newwas called within the 3rd party library (which only the header and a .so was available for). I just simplified the code to make it more obvious (perhaps too obvious).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:
The fix for both bugs looked like this [...]
Thats not a fix, that is proper programming technique! :-D
"We trained hard, but it seemed that every time we were beginning to form up into teams we would be reorganised. I was to learn later in life that we tend to meet any new situation by reorganising: and a wonderful method it can be for creating the illusion of progress, while producing confusion, inefficiency and demoralisation." -- Caius Petronius, Roman Consul, 66 A.D.
jhwurmbach wrote:
Thats not a fix, that is proper programming technique!
Agreed; however, isn't that what most bug fixes are? ;P
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
-
Nishant Sivakumar wrote:
Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.
The leak was not so subtle. However, the memory violation was more subtle in the original code because the
newwas called within the 3rd party library (which only the header and a .so was available for). I just simplified the code to make it more obvious (perhaps too obvious).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:
The leak was not so subtle. However, the memory violation was more subtle in the original code because the new was called within the 3rd party library (which only the header and a .so was available for). I just simplified the code to make it more obvious (perhaps too obvious).
Ah, perhaps it was subtle in the original code. By the way, there's either a leak or a memory violation (since it's the incorrectly placed delete that caused the memory violation). Which of the two was present in your original code?
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 -
Zac Howland wrote:
The leak was not so subtle. However, the memory violation was more subtle in the original code because the new was called within the 3rd party library (which only the header and a .so was available for). I just simplified the code to make it more obvious (perhaps too obvious).
Ah, perhaps it was subtle in the original code. By the way, there's either a leak or a memory violation (since it's the incorrectly placed delete that caused the memory violation). Which of the two was present in your original code?
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 blogThe original code was using Xerces XML parser and calling XMLString::transcode (which you have to read the documentation to realize that it returns a XMLStr* or char* that must be released with XMLString::release). Since the release call was not made, there was a leak (this happened in several places all throughout the code). An object (that resided in another static library) was created and passed the name returned from this transcode function, but the destructor for the object never released any memory (it just held the 2 pointers -- this wasn't initially obvious since the object was in a completely separate library). So when I fixed the memory leak in the library I was working on, we started to see random crashes (access violations) in our simulator. Upon further investigation, the original author of the set of libraries wrote it to store only the pointers, so by fixing the memory leak, I discovered the design flaw. Thus, both were present in the original, but the leak masked the design flaw. Its much easier to see in 30 lines of code or less ... but when you are talking about 5000 lines of code between 3 libraries, its not so obvious.
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 Neither of those bugs were particularly subtle. The leak was glaringly obvious. And to be accurate, there was no 2nd bug in your original code - the 2nd bug was artificially introduced as a possible bad fix to the first bug (the leak) by deleting the memory before the object that had members pointing to that memory went out of scope. Again, that's also not very subtle - pretty obvious bug there.
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:
The leak was glaringly obvious.
Well, all this implicit copying compilers do and stuff like that, i really don't get it, so could you please explain where exactly the leak is ?
-
Nishant Sivakumar wrote:
The leak was glaringly obvious.
Well, all this implicit copying compilers do and stuff like that, i really don't get it, so could you please explain where exactly the leak is ?
Mr.Brainley wrote:
Well, all this implicit copying compilers do and stuff like that, i really don't get it, so could you please explain where exactly the leak is ?
A char array is new'd but there's no corresponding delete.
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