• 13
• 18
• 19
• 27
• 10

Secure/Stable Programming

This topic is 4904 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

Howdy folks, it's da Nurgster here

I've noticed that several threads have a few insecure/instable code segments in them, so I though I'd just give you all a few pointers on improving the dire trend in software development today to leave buggy pieces of elephant feces in apps.

Whenever copying from one area of memory to another, ALWAYS use the size of the destination as the amount of memory to copy, never the source. e.g.:

memcpy(dest, src, sizeof(dest)); // OR
strncmp(dest, src, sizeof(dest));

This simple, and maybe even obvious piece of advise is so foten ignored, and so often the cause of many of the problems involved in computing (especially when it comes to networking).

If you have any other tips, feel free to post them.

Note to felis: I've made this sticky so people will see it, but if you object, I'll make it unsticky.

Share on other sites
I guess i'll just mention the classic ones =)

---

Allways initialize variables when you declare them, even if you're sure you'll attribute a value to them before you use them, you should allways do it.
You can allways add extra code before, and forget to clear it, you can remove that code that initializes it, and forget to clear it in the next piece of code that will use it.

---

When alocating space for an array, particularly strings, allways allocate one more space, like so:

#define STRSIZE 256

char strbuffer[STRSIZE+1];

Even if you define STRSIZE as 256+1, it is allways a good practice to add an extra space, you might change the define later, and that way you allways have a safe guard against buffer overflows(provided you zero the buffer).

^^^

These two when coding relatively large and complex pieces of code, can give you real headache because they don't allways show themselfs.
You might run the program once and it's ok, and after that it crashes, gives out wrong results, or runs for ever. And while you're trying to figure out, what part of the algorithm you implemented wrongly, the mistake is else where, laughing at you =).

---

Sometimes you might use an output funtion to report on the status of a particular part of your code for debugging purposes, like printf, what you can't forget is that some of these funtions are buffered(like printf), meaning, they just output when the internal buffer is full, a speed optimization.
The problem is that, if you try to output a test string to signal that a particular part of your code was executed, again for debugging purposes, the program might crash before the actual string is printed, it remains in the buffer, and then you'll get the wrong impression, you'll think the problem is earlier in the code, because the string that signaled the right execution of the code, didn't show, when in fact was just waiting in the buffer when the program crashed.
To solve this, you need to flush the buffer of the particular output funtion, in the case of printf, a simples fflush(stdout) will do.

[Edited by - White Rabbit on August 21, 2004 2:18:17 AM]

Share on other sites
Quote:
 Original post by NurgleWhenever copying from one area of memory to another, ALWAYS use the size of the destination as the amount of memory to copy, never the source. e.g.:memcpy(dest, src, sizeof(dest)); // OR

Not quite... you want to take the min value of them.
memcpy(dest, src, destsize > srcsize ? srcsize : destsize);

The same really goes for any case where you are copying memory.

Also, when using C++, use the standard containers. It will deal with memory allocations and deallocations for you. Using the bounds safe versions of the accessors can save you a lot of trouble.

Share on other sites
Assert everything. The less likely something will be bad, the more likely you should assert it. The more complex the assert, the more information it will give you. Don't hesitate to write your own assert macro that gives you more information than the standard one does.

You should read articles like: Generic<Programming>: Assertions

Also, refactor everything. The simpler a chunk of code is, the easier it is to debug. It is also easier to test.

Unit test as much as possible. Doing TDD can save you days of debugging, and days more of fixing. The more refined your tests, the better. Remember: A test that should fail but doesn't will often give a better idea of where a bug lies.

Profile last. Then once you've profiled, optimize. But optimize at the highest level possible. Changing the algorithm or data structure you use will generally lead to better results than taking the slow code and making it "faster", assembly should be a last resort. If you have to drop to assembly code, chances are you've picked the wrong algorithm or data structure.

Share on other sites
Quote:
 Original post by WashuProfile last. Then once you've profiled, optimize. But optimize at the highest level possible. Changing the algorithm or data structure you use will generally lead to better results than taking the slow code and making it "faster", assembly should be a last resort. If you have to drop to assembly code, chances are you've picked the wrong algorithm or data structure.

Not entirely true. Sometimes, using assembly is the best way to go. Consider, for example, prefetching. The compiler doesn't know what is good code/data to prefetch, and can't actually make use of this feature too well.

Other things, such as vectorization, would also benefit from assembly, but that requires some work at the initial implimentation to do well. It helps if you have an understanding of what the processor does and when, of course.

Share on other sites
If you use threads, you almost always should also use mutexes! (aka 'critical sections')

Mutexes, or critical sections, or locks, are systems offered by any decent operating system which allow a thread to request and hold sole control of things like global variables and structures. Learn them well, and use them well, curse it!

If you have a single boolean "bFinished", don't worry about it. That is done in a single memory write, and you have no worries.

But if you have a structure holding application properties, or a "to do" list that one thread fills, and another drains... before reading or writing that list, lock it, copy it to thread-specific memory (your heap or stack), and then unlock it.

Why? Because what if, in the draining thread, you've halfway through a memory copy when the threads switch? Now the filler thread writes over the probably unused entry, does its whole copy, and the threads switch again. Now the drainer continues his memory copy... with corrupt data, which is not part of the data he was originally copying! With use of a mutex, the drainer would lock that section. He would start his copy, and then the threads switch. The filler would look, and see the section was locked, and choose to wait until it becomes free. The threads switch again, and the drainer finishes copying the entry, and unlocks the mutex.

There are non-mutex fixes for this particular example, but trust me; you don't want to screw up on something like interthread communication! Go the safe way! Use these programming equivalents of an "Occupied" sign!

Share on other sites
Hmmm... my 'hints' seem a bit low level, compared to the rest. =/

Moderator please feel free to delete my posts if they seem out of context, this one included.

Thanks

Share on other sites
My suggestion, _never_ hide bugs!

Quote:
 Original post by White RabbitI guess i'll just mention the classic ones =)Allways initialize variables when you declare them, even if you're sure you'll attribute a value to them before you use them, you should allways do it.You can allways add extra code before, and forget to clear it, you can remove that code that initializes it, and forget to clear it in the next piece of code that will use it.---When alocating space for an array, particularly strings, allways allocate one more space, like so:#define STRSIZE 256char strbuffer[STRSIZE+1];Even if you define STRSIZE as 256+1, it is allways a good practice to add an extra space, you might change the define later, and that way you allways have a safe guard against buffer overflows(provided you zero the buffer).

It's just hiding bugs - usual practice for C/C++ programmers. Everything is done in that way, "just hide bug,and everything is OK now" :( .Adding extra space it's not avoiding buffer overflow but making buffer overflow to happen less frequently
edit:on normal usage. It does not protect against hacker.

Language that can't optionally check array bounds are actually extremely crappy and shouldn't be used in any critical or networking application. But it's used!
(extra time to check array bounds is essentially _zero_ for most applications where it's more-or-less critical(networking,etc). Networks are VERY slow comparing to computers,even 100Mbit ones.)

Quote:
 ---These two when coding relatively large and complex pieces of code, can give you real headache because they don't allways show themselfs.You might run the program once and it's ok, and after that it crashes, gives out wrong results, or runs for ever. And while you're trying to figure out, what part of the algorithm you implemented wrongly, the mistake is else where, laughing at you =).

Share on other sites
here is one that not many people realize when using STL containers:

When using STL containers, always explicitly specify copy constructors and/or copy operators!

sample of safe STL structs/classes:

class MyGenericClass { public:  MyGenericClass( const MyGenericClass& rhs ) {     (*this) = rhs;  }  MyGenericClass& operator=( const MyGenericClass& rhs ) {     m_A = rhs.m_A; // has assign operator defined     // 'SomeClassType_WithOutCopyOperator' doesn't have assign operator so do it manually!     m_B.m_someMember = rhs.m_someMember;     m_B.m_someOtherMember = rhs.m_someOtherMember;     return (*this);  } public:  SomeClassType_WithCopyOperator m_A;  SomeClassType_WithOutCopyOperator m_B;};

RULE OF THUMB:
Use this mainly for large classes with non-primitive member variables.

in somecases, it may be better (read: easier) to let the
compiler implicitly define a member-for-member default copy
constructor/operators.

Share on other sites
Quote:
 Original post by silvermaceMyGenericClass( const MyGenericClass& rhs ) { (*this) = rhs; } MyGenericClass& operator=( const MyGenericClass& rhs ) { m_A = rhs.m_A; m_B = rhs.m_B; return (*this); }

I wouldn't do that in the copy constructor

(construction/initialization) != assignement

even thou the final result is the same they are not the same operation, with assignement the variable always had "some" previous value that is replaced.

Also like to add always prefer the default copy/assignment operations implicitly defined or if doesn't make sense for a type to copy/assign then make them protected/private. Only define what means to copy & assign when you have data members that are raw pointers.