Manage risk of "Overflowing fixed-length string buffers"

Question

I want to verify that I'm correctly handling risk of "Overflowing fixed-length string buffers." I'm also aware that I'm using C-style strings, which falls short of full C++ code.

Main

/*
PURPOSE:
attempt to implement, without using given/known-good code, various concepts
 of c++.
one exception:
code in header fileToArray/set_cArrayTemp()
*/
//MOVED TO TOP - ELSE MY HEADERS FAIL
//boilerplate - won't have to use: std::
using namespace std;
//needed to use cout/cin
#include <iostream>
//for using system calls -
//warning: (www.cplusplus.com/forum/articles/11153/)
#include <cstdlib>
//to use strlen
#include <cstring>
//c++ headers (e.g. iostream) don't use .h extension so i am not either
//provides indentation and v spacing for logging
#include "header/logFormatter"
//reads a file to a char array
#include "header/fileToArray"
//dialog gets filename from user
#include "header/fileDialog"
//this is the (p)array that should hold all the text from the file
char *cArray;
//the name of the file to read
char *fileName;
void set_fileName(){
 cout << vspace << col1 << "begin set_fileName()";
 char *temp = getFileName();
 cout << col2 << "tmp: " << temp;
 fileName = new char[strlen(temp)];
 strcpy(fileName,temp);
 delete[] temp;
 cout << col2 << "fileName is set: " << fileName;
 cout << col1 << "end set_fileName()";
}
void set_cArray(){
 cout << vspace << col1 << "begin set_cArray()";
 char *temp = fileToArray(fileName);
 if(temp){
 cout << col2 << "tmp: " << temp;
 /*FROM MAN STRCPY:
 "If the destination string of a strcpy() is not large enough,
 then anything might happen. Overflowing fixed-length string
 buffers is a favorite cracker technique for taking complete
 control of the machine."
 */
 //so, this guards against overflow, yes?
 cArray = new char[strlen(temp)];
 strcpy(cArray,temp);
 delete[] temp;
 cout << col2 << "cArray is set: " << cArray;
 cout << col1 << "end set_cArray()";
 return;
 }
 cout << col2 << "fail - did not set cArray";
 cout << col1 << "end set_cArray()";
}
//expect memory leaks = 0
void cleanup(){
 cout << vspace << col1 << "begin cleanup()";
 if(cArray){
 delete[] cArray;
 cout << col2 << "cArray deleted";
 }
 if(fileName){
 delete[] fileName;
 cout << col2 << "fileName deleted";
 }
 cout << col1 << "end cleanup()";
}
void closingMessage(){
 cout << vspace << col2 << "APPLICATION COMPLETE";
}
int main(){
 system("clear;");/*yes, i know (www.cplusplus.com/forum/articles/11153/)...
 ,but it provides focus for the moment and it is simple. */
 //col0
 cout << "begin main()";
 set_fileName();
 set_cArray();
 cleanup();
 closingMessage();
 cout << "\nend main()";
 return 0;
}
//TODO - 
//1. use a doWhile so that user can run once and read many files without
//app exit.
//
//2. find way to provide init (not null/safe sate) of pointers

header (fileToArray):

 //reads a file to a char array
//for using files
#include <fstream>
// user inputs a file [path]name
char *filenameTemp;
//this is the (p)array that should hold all the text from the file
char *cArrayTemp;
void set_filenameTemp(char *fileName_param){
 cout << vspace << col3 << "begin set_filenameTemp()";
 filenameTemp = new char[strlen(fileName_param)];
 strcpy(filenameTemp,fileName_param);
 cout << col4 << "file name assigned: " << filenameTemp;
 cout << col3 << "end set_filenameTemp()";
}
void set_cArrayTemp(){
 cout << vspace << col3 << "begin set_cArrayTemp()";
 ifstream file(filenameTemp);
 if(file.is_open()){
 /*---------------------------------------------
 source: www.cplusplus.com/doc/tutorials/files/
 modified a bit*/
 long begin,end,fileSize;
 begin = file.tellg();
 file.seekg(0,ios::end);
 end = file.tellg();
 fileSize = (end-begin-1);/* -1 because testing shows fileSize is always
 one more than expected based on known lenght
 of string in file.*/
 /*---------------------------------------------*/
 //cout << col4 << "bytes in file (fileSize=): " << fileSize;
 cArrayTemp = new char[fileSize];
 file.seekg(0,ios::beg);
 file.read(cArrayTemp,fileSize);
 file.close();
 cout << col3 << "end set_cArrayTemp()";
 return;
 }
 cout << col4 << "fail - file not open";
 cout << col3 << "end set_cArrayTemp()";
}
//caller is responsible for memory
//may return null
char *fileToArray(char *fileName_param){
 cout << vspace << col2 << "begin fileToArray()";
 if(fileName_param){
 cout << col3 << "file name received: " << fileName_param;
 set_filenameTemp(fileName_param);
 set_cArrayTemp();
 delete[] filenameTemp;
 cout << col2 << "end fileToArray()";
 return cArrayTemp;
 }
 cout << col3 << "received NULL fileName_param";
 cout << col2 << "end fileToArray()";
 return cArrayTemp;
}

header (fileDialog):

 //dialog gets filename from user
// user inputs a file [path]name
//number of chars to get from user input
static const int size = 20;//20 will do for now
/*FROM MAN STRCPY:
 "If the destination string of a strcpy() is not large enough,
 then anything might happen. Overflowing fixed-length string
 buffers is a favorite cracker technique for taking complete
 control of the machine."
*/
//so, this guards against overflow, yes?
//that is, by using getline, rather than cin >> var,
//size of array is controlled.
//caller is responsible for memory
char *getFileName(){
 cout << vspace << col2 << "begin getFileName()";
 char* input = new char[size];
 cout << col2 << "enter filename: ";
 cin.getline(input,size);
 cout << col2 << "end getFileName()";
 return input;
}
//NOTE:
// currently, this hardly justifies a header file - i expect to do more later.
// may eventually use this for all userdialog and rename to userDialog

header (logFormatter):

 //STRICTLY FOR LOGGING
// spares me from managing chains of \n and \t
//this allows me to use indentation to show progress/flow
// making this a psuedo-debugger
//
//left-most column - col0 is imaginary/conceptual
//char col0 = "";
static const char col1[] = "\n ";//4 spaces
static const char col2[] = "\n ";//8 spaces
static const char col3[] = "\n ";// etc.
static const char col4[] = "\n ";
static const char vspace[] = "\n\n";
//NOTE: changed from using \t since default is 8 spaces

Answer 1

It is very apparent that you are new. That is fine; we were all new once. I'll try to adjust my feedback accordingly. Let me know if you are not familiar with some of the terminology, and I'll provide an explanation or definition.

---

Overflow safeguards

First of all, I'll discuss what seems to be the issue you are most concerned with: buffer overflows. I'd like to state that at this level, you should not really care about that (yet), at least not from a security standpoint. You should focus on learning the language and programming in general first.

Your code is following the general correct idea: Make sure buffers are large enough by dynamically allocating memory, and limiting the size of input strings when you are not. Note that std::string does all of this for you by growing as needed.

In modern C++ code, you would normally avoid allocating buffers the way you do, because memory management quickly becomes hard as a program grows. In C++, the RAII pattern is essential. It boils down to allocating resources in the constructor of an object (a class instance), and freeing them automatically in the destructor when the object goes out of scope. This is what std::string does for you, as well as growing as needed if you add text to the string.

---

High-level issues

1. I strongly recommend you to reduce the commenting level.

I used to teach programming at the local university, and I saw that over-commenting "technique" a lot. My experience is that it not a good idea. It works as a crutch, allowing you to read your comments rather than your code. However, you already know how to read text; you need to learn how to read code. Stick to regular commenting levels. If you must have notes, keep them in a separate document. You want to make it as inconvenient as possible for you to look at them, forcing you to read the code itself when possible.

2. You are not writing C++.

You are writing C code, with C++ library calls. Write your own String class instead of using raw arrays in the code. Take advantage of all the things C++ has to offer. (This normally includes std::string, but writing your own String class for practice is a nice exercise.)

3. Your headers should not contain function definitions.

In C++, there is something called the one definition rule , which states that any definition should occur at most once in a program. (There are some exceptions to this, but you don't have to think about that yet.) Headers are meant to be included in several files, so they can only have declarations in them. For example:

In fileDialog.hpp:

char *getFileName();

In fileDialog.cpp:

char *getFileName(){
 cout << vspace << col2 << "begin getFileName()";
 char* input = new char[size];
 cout << col2 << "enter filename: ";
 cin.getline(input,size);
 cout << col2 << "end getFileName()";
 return input;
}

The former is a declaration, the latter a definition. While we're on the subject of headers: It's normal for user-defined headers to have a .h, .hpp or .hxx suffix. I personally prefer .hpp to separate them from C headers.

4. Avoid using global variables.

Global variables are bad, because they have a very large scope and can be changed from anywhere, at any time, sometimes without you realizing. Either implement a class design and put the variables into class scope, or pass them around using function arguments. Variables that will never change (often called constants :-) ) can be left in the global scope, but should be declared const.

5. Learn the basics of a debugger.

Basic use of a debugger is very simple, and it allows you to remove a lot of the cout calls that clutter the code. Learn to set breakpoints and step through your code; that's all you need for now. As a beginner, I recommend using a visual debugger and not just raw gdb. (You can use a gdb frontend, though.)

6. Separate output from computations.

Functions that do something should generally not perform IO. One of the key reasons for that is reusability. You want to write code that you can reuse later. While it's not very likely that you will reuse these functions later, you should train as you fight and follow good practice whenever possible. Later users of your functions (i.e. you at a later time) may not want that output, and the way to solve that is to decouple IO from computations.

---

Lower-level issues

7. It's safe to delete a null-pointer.

Instead of writing

if(cArray){
 delete[] cArray;

Write

delete [] cArray;
cArray = nullptr;

deleteing a null-pointer has zero effect, and is therefore harmless, so there's no point in checking against null. What you should do, however, is to set your pointer to null after deleting it, ensuring that nothing bad will happen if it is deleted again. In C++11, the null pointer is called nullptr. If for some reason you are not using C++11 (as a C++ learner in 2013, you should be), use 0 (or NULL) instead.

8. Consider inverting conditions to reduce nesting.

Instead of this code (superfluous comments and couts removed, whitespace inserted to increase readability):

if (temp) {
 cArray = new char[strlen(temp)];
 strcpy(cArray,temp);
 delete[] temp;
 return;
}
// Handle temp == nullptr ...

Consider writing this:

if (!temp) {
 // Handle temp == nullptr ...
}
cArray = new char[strlen(temp)];
strcpy(cArray,temp);
delete[] temp;

There is a lot more to comment on, but these are the most pressing issues for now, and should be more than enough to get you started. I encourage you to implement as many of these changes as you can (except maybe refactoring to classes), and then post your updated code as a new question for further review.

Some of the things that still remain to be pointed out are:

• Best practices
• Design issues -- what I would do differently, and why
• Identifier naming
• Exception safety and memory leaks
• File IO

(I am listing these so you can think about them yourself before posting another review.)

• Jamal: +1. I did not think about the commenting that way, but I also didn't want to sound too arrogant. I've learned some other things, too.
• yas: @Lstor - First, thank you for the time you have given me. Second, thank you for info on nullptr - I've read a fair amount in a few weeks and found that nowhere. First step will be to abandon gedit in favor of an IDE to get debugging. Then I work my way through all that you have provided.
• Lstor: You're welcome :-) To get C++11 with g++, compile with -std=c++11. You don't have to use an IDE, you can also use a standalone visual debugger. Personally I prefer Vim and Nemiver for small-scale development.
• chux: cArray = new char[strlen(temp)]; strcpy(cArray,temp); is bad - UB as not enough memory was allocated for a string. Suggest cArray = new char[strlen(temp) + 1];.

Answer 2

• You may not like to use std::, but it'll save you some headaches eventually. I too was in the habit when I started, but I broke out of it. More information about its use here.

• You really won't need closingMessage(), especially since it only outputs one message. It's an unneeded function-call, too.

• size can just go inside getFileName() if there will be no other functions to use it. Better yet, just put that function in the same file as main(). This also applies to logFormatter.

• I will have to agree with @Lstor on the commenting thing. Although there's nothing wrong with documenting your code, too much of it creates clutter. At this level, I would just comment like this:

  1. Include a short explanation of the file at the top. This will remind you AND others what the following code is supposed to do.

  2. If a line or block of code may look complicated to someone else, add a few notes about it. If you're just doing something common like file I/O, no need to comment that. YOU may not know how it works right away, but that's why you're learning through coding.

• Although @Lstor has mentioned std::string already, I cannot stress it enough. You're do a lot of deleting pointer-handling here. Don't be afraid to learn the STL; it's there to help you. Yes, you should learn about deleting and pointer-handling on your own, but that would be best for "learning" programs and not serious programs. You cannot let this get in the way of learning responsible coding, especially when other people read your code.

• Lstor: "If a line or block of code may look complicated to someone else, add a few notes about it." Better yet, rewrite it so that it doesn't look complicated, or hide it inside a function with a meaningful name. If that's impossible, then comment it. I like to have very short comments describing 5-10 line blocks of code, though, almost like labels.
• yas: @Jamal - as I said to Lstor, thank you for your time! I expect a request for code review is more demanding than a request to answer a focused, one point question. I really appreciate the link you provided concerning namespace std; that causes immediate change.