how to make sure that a function won't get a garbage pointer?

Question

I have a function that accepts a pointer and return an enum depending on some conditions related to that pointer:

my_enum function(char* prt)
{
  /* function body*/
  if (condition1) return enum1;
  if (condition2) return enum2;
  if (condition3) return enum3;
  if (condition4) return enum4;
  else return enum5;
}

I have another function which also accepts a pointer, invokes my_function and reacts to the obtained value:

void another_function(char* ptr)
{
 my_enum result = function(ptr);
 if (result == MY_VALUE) std::cout<<"OK"<<endl;
}

I'm running Valgrind to check for memory leaks. The above code results in the following error:

Conditional jump depends on an uninitialized variable.

In fact, it is possible to pass an uninitialized pointer to the function function.

My question is: What is the best way of dealing with this situation (apart from using references instead)? I can't make sure that everyone who will use that code, will initialize the pointer that he will pass to the function. I can't check inside my function if the pointer points to some garbage (I'm checking whether it is a null pointer though) as well.

Should I ignore such errors? If they are useless, why does Valgrind bother to inform me about them? There must be something I can do.

Answer 1

How far are you willing to go? If someone WANTS to break your code, they will, you can't help it.

The more efficient protections you apply the more difficult they get.

The one simple one is to check for NULL. That doesn't prevent stupid pointers, but it prevents ones consciously invalidated. Most people are satisfied by that.

Then you may give the pointer a wrapper class. Instantiating this class requires a valid object pointed to (or some hopeless jumping through hoops to give it an invalid one, which amounts to purposefully shooting your foot), so no scenario of uninitialized pointer can occur - but the object can cease to exist before its pointer is used.

Then you can maintain a factory/manager class for these objects and their pointers. Every time the pointer destination object is created or destroyed, its pointer is created or invalidated. This will be fail-proof unless your code is multi-threading and destruction can occur while your function is already past the checks and before using the validated value.

Then you can add thread safety, wrapping both your function and the manager in mutexes. This adds all kinds of headaches related to deadlocks and synchronization. But the user must really try very hard to create a class derived from yours (probably with #define private public first) that overrides its safety features...

With each step your overhead climbs to levels where the effect really stops being worth the effort. So just check that pointer for NULL and stop worrying about others out to get you.

Answer 2

Opinions will vary on what is the "best" approach, since it is impossible to prevent someone passing a bad (e.g. uninitialised, dangling) pointer at all.

A common solution is to avoid raw pointers altogether, and write the function in a way that does not accept a pointer at all.

One way is to accept a reference. Writing your code so it doesn't use raw pointers at all makes it harder to call your function with a bad parameter. The limitation is that the caller can still create a bad reference (e.g. by dereferencing a bad pointer) but it takes more effort (or a longer sequence of mistakes if done unwittingly) to pass a bad reference to a function than it does to pass a bad pointer.

Another way is to accept some class object by value (or reference, in some cases) to hold your pointer. Then implement all member functions of that so that they prevent a situation of holding a bad pointer. Give that class no member functions that accept a pointer. Ensure the constructors and other member functions maintain consistency (formally, the constructors establish a rigorous set of invariants, other member functions maintain that set of invariants). This includes techniques like throwing an exception if an attempt is made to construct an object using bad data (if an exception is thrown in the process of constructing an object, that object never exists, and cannot be passed in any manner to your function). As a result, your function can assume - if it is successfully called - that the data it receives is valid.

The thing is, the above make it harder to accidentally pass bad data to your function. No technique can absolutely prevent someone who is determined enough (whether through genius or foolishness) to find a way to bypass all the safeguards, and to pass bad data to your function.

Answer 3

There are essentially two solutions.

1. Expect a valid pointer and state that clearly in the documentation of your API. Then any invalid use will cause UB, but it's not your fault. However, handling raw pointers is C-style and frowned upon by C++ programmers.

2. Take (the reference to) an encapsulated pointer type, which is always sensibly initialised, such as std::string (instead of const char*), std::unique_ptr, or std::shared_ptr. For example,

   my_enum function(std::string const&str)
   {
     /* function body*/
     if (str.empty()) // deal with improper input
       std::cerr<<"warning: empty string in function()"<<std::endl;
     if (condition1) return enum1;
     if (condition2) return enum2;
     if (condition3) return enum3;
     if (condition4) return enum4;
     else return enum5;
   }
   

   or

   my_enum function(std::unique_ptr<SomeType> const&ptr)
   {
     /* function body*/
     if (!ptr) { // deal with improper input
       std::cerr<<"warning: invalid pointer in function()"<<std::endl;
       return enum_error;
     }
     if (condition1) return enum1;
     if (condition2) return enum2;
     if (condition3) return enum3;
     if (condition4) return enum4;
     else return enum5;
   }
   

   This avoids raw pointers and is the C++ way for dealing with this sort of situation. One problem with the latter code is that it only works for unique_ptr arguments. One may generalise this to be overloaded (using SFINAE or otherwise) to take (const reference to) any auto-pointer like objects (for instance defined as objects obj with member obj::get() const returning a const obj::element_type*).