1

Couldn't find the answer by serching (maybe bad keywords), so I am creating a new question.

How do you handle parameter checking for dllexported methods with string parameters. The general rule is never trust user, but in reality? For example:

int foo(const char *bar)
{
    if(!bar)
        return FAIL;

    ???
}

Say the user of the library calls our function like:

foo(reinterpret_cast<char*>(0x00000008));

That should cause an AV on first:

strlen(bar);

Is there a way to guard against this? Correct approach to handle the error?

I know IsBadReadPtr is out of the question, because this function is in a class of dangerous and never to be used. But is there even a way I should and could handle the problem? I can't __declpec(dllexport) std::string, can I? Moreover, even if I would, the std::string has some sort of thread local storage or statics that cause access violations when used from different modules, as far as I know (caused by statics or different heaps?).

Is there a security risk in using these functions, stack overflow (R/E)IP overwrite, or is it just going to cause safe AV?

Coder
  • 3,695
  • 7
  • 27
  • 42

9 Answers9

8

You can only babysit the client so far. If they pass garbage to your function, is it your problem?

wallyk
  • 56,922
  • 16
  • 83
  • 148
5

"Never trust the user" is a saying that applies in a different context - when the "user" is a person who runs a client program and sends data to a server program that you are writing.

When the "user" is someone who is using your code as a library, it should be closer to: "Always write your code so that anything that goes wrong really is the user's fault; then blame the user if something does go wrong". :)

Karl Knechtel
  • 62,466
  • 11
  • 102
  • 153
2

Your code's responsibility is its own. It is the client's responsibility to error check their own data that they're going to input. Additionally, there is little you can do to ascertain the validity of a valid pointer. Does it point to non-null? Then that's about as much as you can check.

If you are concerned about security, best you can do is catch & handle/throw exceptions and/or return error notifications of some sort. Pointers inherently have a degree of danger to them. Static-typing won't save you from invalid pointers and access violation.

Sion Sheevok
  • 4,057
  • 2
  • 21
  • 37
1

You might want to change the function signature to include the length of the string. If you're reading from the string, it'll mean that your program will not continue reading beyond the string if it was not correctly NULL terminated. If you're writing to the string from your function, then you must take the length of the buffer as a parameter and you must check that you do not write more data than the buffer can hold.

Wallyk's response is good to have in mind. If your user passes garbage into your function, there's only so much you can do to fail gracefully.

CadentOrange
  • 3,263
  • 1
  • 34
  • 52
0

You can probably use OS functions to test if the address is mapped to real RAM address. But this solution only half-solved your problem. Because even you know the memory address is valid address, you still don't know whether users are passing existing but wrong address to you.

They can simply pass, foo( (char*)&very_important_variable);

So what can you do then? You can't really do anything I guess.

user534498
  • 3,926
  • 5
  • 27
  • 52
0

Is the DLL one that you use only in your workplace or one you ship externally? If it is one you use only internally using std::string in it is generally safe nowadays because all your code will be built and linked against the same runtime library. The danger in std::string otherwise is breaking the "one-definition rule". Same with boost::shared_ptr etc. But we want to be able to use those in our C++ libraries.

The best option usually to defend against illegal NULL pointers is an assert but if you are shipping and your clients will not get debug version that won't help.

CashCow
  • 30,981
  • 5
  • 61
  • 92
0

The above posts have said most of it. You basically can't fully protect yourself from client code. The client could just use OpenProcessMemory and just mess up all your internal data structures.

For debugging purposes you can definitely use IsBadReadPoiner. The only caveat here is that once the guard page triggered you kind of should shut down (gracefully if possible) your application and start over. One thing I like to do is put a big and ugly message box in the debug build that pops up if IsBadReadPoiner is hit.

rioki
  • 5,988
  • 5
  • 32
  • 55
0

I'll take against the popular approach here, saying that a) a library should be robust (to a reasonable extent) against client's errors, and b) when a program crashes with AV in your code, you look bad, even though you can point to a line in the manual and say "told ya!". This is a politician, rather than software engineering, attitude...

Not being Windows programmer I can't give a specific advise, but seems that at least a return code or an exception are appropriate here.

davka
  • 13,974
  • 11
  • 61
  • 86
-1

There is no generically applicable way to guard against this; C/C++ do not have a generic pointer-validation facility.

As you've shown, you can fence against NULL pointers but not (easily) against invalid ones:

template <class T> bool isValid(T* ptr) { return ptr != NULL; }
class X {
    int someVal;
    int getSomeVal(void) const { return isValid<X>(this) ? someVal : -EINVAL; }
};

// the following will set 'j' to -EINVAL
X* nullX = (X*)NULL;
int j = nullX->getSomeVal();

// the following will make getSomeVal() crash:
X* invalptr = reinterpret_cast<X*>(0xdefeca7ed);
int i = invalptr->getSomeVal();

Some compilers / libraries / heap allocator classes (optionally) zero-initialize all heap memory and hence on such platforms, "uninitialized" heap contains zeroes - on such platforms the above is useful to guard against the use of uninitialized heap. But that costs performance and therefore isn't mandated by the C / C++ standards. And it doesn't help with uninitialized temporary objects (that are created on the stack not on the heap, and hence cannot be zero-filled by the heap allocator).

To guard against any stray pointer, you'd have to implement your own allocators, and provide hook calls (a much more elaborate version of isValid() above) allowing an object instance to query the allocator state regarding "hey allocator have you seen my this before ?", and make all class constructors for stack-based objects (de)register the object instance address with the "validity tracker" on creation/deletion. It'd be a very heavyweight approach to search the heap allocator maps each time any object method is called. I've not seen this in production practice, though some debugging tools (valgrind comes to mind) have allocator tracking, and some limited stray detection.

And if as a user of the library you desperately want to cheat, what stops you from doing:

X* objX = new X();
Y* objY = reinterpret_cast<Y*>(objX);
Y->someMethodThatAccessesThingsWayOutsideTheSizeOf_X();

Even if that method would validate this with the allocator, it'd find it ok/known but how would you tell that it "really is an Y" there ?

In general, a library's resilience depends, to a large degree, on the user. How much nannying is too much ?

FrankH.
  • 17,675
  • 3
  • 44
  • 63
  • `0xdefeca7ed` can be a valid pointer in Windows, with the `LARGEADDRESSAWARE` flag. Furthermore, the `isValid(this)` test can be optimized out because `this` is never NULL, by definition. You need to turn off the optimizer for that part. Why is it even a template? All pointers convert to `void*`. – MSalters Dec 08 '10 at 14:54
  • I don't think you know what you're talking about when you claim that `this` cannot be NULL. `this` being NULL is a very valid situation, and in fact one can use it deliberately. You're correct that one can have `0xdefeca7ed` as a valid address (on x86), if your application is just allocating enough memory. Most non-x86 architectures will always trap at that pointer though - because it's misaligned. – FrankH. Dec 08 '10 at 18:07
  • To clarify, `this == NULL` is possible for nonvirtual member functions. I've yet to see a compiler that'd reject it. See also http://stackoverflow.com/questions/2679080/this-null-how-can-it-be-possible – FrankH. Dec 08 '10 at 18:14