24

Assuming I have the following struct (just an example)

struct string{
    int len;
    char*str;
}

And I have the function

int init_str(struct string*s, int len);

which will perform

s->len=len;
s->str=malloc(len*sizeof(char))

And then I have the function

int do_something(struct string*s, ...)

which will assume s->str is a validly malloc()'ed pointer

Should I assume that s->str is a malloc()'ed pointer set by init_str()?

10
  • 17
    Does the C language even give you a way to confirm that a pointer you receive is valid? I didn't use C in quite a while and I can hardly claim to be an expert in the language, but as far as I remember it was a huge problem in that language that you just have to trust the caller that pointers you receive point to properly initialized and allocated memory.
    – Philipp
    Dec 2 '21 at 8:14
  • You should probably check that len is bound properly first. Not < 0 and not too large. Dec 2 '21 at 14:48
  • 2
    It can't be negative, though, which you currently permit. You should declare the len argument to init_str and the len element of string as size_t to guarantee that they cannot hold an invalid value. (As an aside, sizeof(char) is defined to equal 1, so you can just say malloc(len).)
    – Ray
    Dec 2 '21 at 22:56
  • 4
    Wouldn't you wish you were programming in a more modern language where objects can promise to be always sensibly initialized, where malloc is a thing of the past and where you don't have to reinvent the wheel every time you try to do more than a simple computation... Dec 3 '21 at 13:31
  • 3
    @Harf, sizeof(char) is always 1 (see e.g. en.cppreference.com/w/c/language/…), but char doesn't need to be 8 bits.
    – ilkkachu
    Dec 3 '21 at 14:46
14

If there is any way to be sure that the struct is valid, then go ahead and assume it is valid.

However, if you want the application to be robust, you should assume nothing - and just check if everything is OK.

Note: one way to ensure that the structure is valid is to know that another function is called before "do_something()" and that function validates everything. Even so, it is dangerous, because that other function might change in the future, and remove those checks - leaving your function to operate on non-validated data.

Note: the discussion might continue based on how you plan to use the data in the structure, but that is pretty much out of the scope of the question.

Note: from an extreme POV, the structure is always valid. You have a starting address and a length, and therefore you have a "string" (actually a buffer). If that string is relevant to you, or if you should (or should not) modify that string, is another discussion :)

Note 2: as a matter of fact, you cannot really validate a pointer. The only invalid pointer is NULL, but even then things can get tricky. I used in the past a microcontroller, and port A was physically mapped to address 0x00. So reading port A could actually done by the most wrong thing to do in C: de-referencing the NULL pointer. We had to hack the results of the static analysis tool to filter out the statements involving port A, before the remaining results became readable.


Should I assume that s->str is a malloc()ed pointer set by init_str()?

If you read the documentation properly, you will see that even "malloc()" itself can fail - and in that case it will return error codes. So even if you make sure that "malloc()" is called, you cannot know at the time of writing the software if the call to "malloc()" wil succeed or if it will fail. So verifying the return value of "malloc()" and designing the subsequent code based on the returned value is the safe way to go.

4
  • 2
    It may not really matter for this answer, but AFAIK malloc does not "return error codes" when it fails, it returns a null pointer.
    – Doc Brown
    Dec 3 '21 at 15:07
  • I did not use "malloc()" in a long time, and I was lazy to re-read the documentation :D However, "null" is as good as an error code, actually. Good point, though.
    – virolino
    Dec 3 '21 at 17:15
  • 1
    Don't play this game. You cannot validate if str points to a heap buffer or not. You can check for NULL, and on most platforms a slightly more robust check is possible, but don't bother validating for wild pointers. Just don't. It's all pain no gain.
    – Joshua
    Dec 3 '21 at 21:16
  • 4
    This is pretty poor advice. You can’t check for pointer validity in a portable way, this is a fools errand (there are some tools like ASAN for some implementations that can catch issues though). What you should be doing is encapsulate the data and make sure none of the functions that do directly access the data can leave it in an invalid state.
    – Cubic
    Dec 4 '21 at 0:31
66

If you are at a security-boundary, you must assume malicious use.
Check everything.

If you are at a module-boundary, you might anticipate programmer error.
Try to catch what you can without compromising your performance and space-use goals, allowing a higher toll if compiled for debugging.

If you are writing module internals, too much checking will just get in the way. On the other hand, too little will make finding and diagnosing bugs unnecessarily hard.


Above all else, remember your goals, both functional and non-functional, and exercise due diligence and common sense, as uncommon as it may be.

3
  • 6
    "allowing a higher toll if compiled for debugging" - In particular, be aware that any asserts will be compiled out if you #define NDEBUG. So feel free to sprinkle assertions all over the place; they won't slow things down at all in the final version.
    – Ray
    Dec 2 '21 at 22:59
  • 4
    Good assertions are at least as valuable for their documentary characteristics as for their bug-signalling capabilities. But, of course, they must not be mistaken for a means to perform essential validation. Dec 3 '21 at 5:47
  • 1
    This. Choose the right level of paranoia according to how the code will be applied. Checking everything for every possible condition is a good classroom exercise, but not always appropriate in real applications.
    – wberry
    Dec 4 '21 at 0:32
17

There are a couple of different ways to go at this.

  • The function should clearly document its expectations about the provided data.

  • The function should employ defensive programming and validate necessary properties internally.

  • It should not be possible to represent invalid states.

However, you're writing C, so things are difficult.

In C, documenting the expectations is probably the most reasonable approach. For example, the init_str() might have the expectation that s points to valid but uninitialized memory (i.e. it can write but not read the values). Or it might have the expectation that s points to zeroed-out memory. Or it might allow s to point to an existing string, which would be freed and then re-initialized.

Depending on the expectations, there is no reasonable validation you can do. How could you tell whether a string is valid or not, except maybe by checking that the pointer is non-null?

Using the type system to make invalid states unrepresentable is something that's very nice in C++. In C, it is a lot trickier since there is no concept of “private” data. However, it is possible to get some degree of encapsulation via incomplete types. Consider this string interface in a header file:

typedef struct string string;
string* str_new(size_t len);
void str_free(string* s);

It is not possible to create a string except through the str_new() function, as string is an incomplete type.

The implementation might then be:

struct string {
  size_t len;
  char data[];  // flexible array member
};

string* str_new(size_t len) {
  if (len > SIZE_MAX - sizeof(string)) return NULL:
  string* s = malloc(sizeof(string) + len);
  if (!s) return NULL;
  s->len = len;
  return s;
}

void str_free(string* s) {
  free(s);
}

It is however possible to obtain a string in an invalid state, such as with a use-after-free.

In some cases tricks with incomplete types are worth it, in particular if all-zero is not a good default value for a type. But for a string, probably not. I would either document the function's expectations clearly, or make the business case to switch to C++ which has a much stronger type system that allows for much better safety.

7
  • 2
    string* s = malloc(sizeof(string) + len; That flies in the face of being robust. Also, it is off by one, as normally you would want a nul-terminator even if it is truly a counted string, as long as the cost is not too big. Dec 2 '21 at 0:43
  • @Deduplicator I added a check for numeric overflow. Whether an extra null byte is needed depends on how that string is used. For example, I my example code doesn't zero out the string contents so it's not usable as a C-string anyway, just as a buffer.
    – amon
    Dec 2 '21 at 9:44
  • As an aside, if you pointed to the string-data (not the length before it) and added a terminator, it becomes quite similar to a BSTR. Dec 2 '21 at 21:41
  • "It is not possible to create a string except through the str_new() function," -> string *s = malloc(1); // yay C implicit pointer conversions.
    – TLW
    Dec 4 '21 at 20:29
  • Also, this approach often adds a layer of indirection (you can't stack-allocate the type any more). A better approach in many cases is to (ab)use the preprocessor to make the fields within the struct inaccessible (struct string_t {string_data_t _string_internals;}; #define _string_internals PLEASE_DO_NOT_USE_STRING_INTERNALS_DIRECTLY (with an undef in the actual string implementation).) You can get around this (as always in C), but you're rather unlikely to accidentally do so.
    – TLW
    Dec 4 '21 at 20:33
3

In my experience, its best to rephrase the idea of "assume s->str is a valid malloc'd string" into one of the phrasings which captures not just the assumption, but what happens if it fails.

The weakest notation I'd recommend is using the term "preconditions." Preconditions are things that must be met in order for the algorithm to work. Saying "Preconditions: s->str is a valid malloc'd string" is the formal way of saying that you're assuming it to be true. Specifically it states that you're really not considering what happens if s->str is not a valid string. If you use "assume," it may be ambiguous (with the different meanings below), but preconditions is a formal phrasing with a well understood meaning.

If we want to go further, the next step is to try to explain what happens if the pre-conditions are not met when the function is called. Fortunately, there are some accepted wordings that can be used for this. The first on the list is "undefined behavior." This is the nastiest of these formal wordings. If I state that "if s->str is not a valid string allocated with malloc() the behavior of the function is undefined," that has a clear meaning of "absolutely anything can happen." In your case, this is likely the behavior you will see. If you access memory that isn't allocated, C++ calls that undefined behavior. Undefined behavior is really bad news. It can cause any effect, including affecting completely unrelated code (such as writing data over the top of other functions' data). If it crashes when you do, that means you got lucky. If it gives you wrong answers several seconds later in an unrelated function, it may take a long time to figure out what happened.

A kinder guarantee you can offer is "unspecified" behaviors. Unspecified behaviors are those which aren't reliable, but will always at least be limited to a known set. A function that returns an "unspecified integer if s->len is greater than 1 million" can return any integer in that case, but the rest of the behaviors are understood. Unspecified behavior won't create unexpected surprises in other sections of code... they'll just give an unspecified result.

The most well known use of unspecified behavior in C++ I am aware of is the order of evaluation of arguments. In f(g(), h()), it is unspecified what order g and h are executed in. But you know for certain that either g is executed and then h, or h is executed and then g, and you can plan accordingly. In my own programming experience, I've often used unspecified results to describe what happens when operating on something with 0 elements. Lots of algorithms are well defined for a non-zero number of elements, like a f(1) or f(15), but there just isn't a really good answer for what should happen for f(0) because the math just sort of breaks down. In these cases, I will often elect to say "the result of f(0) is unspecified," and let it be whatever behavior my implementation happens to have (as long as it doesn't invoke any undefined behavior).

One step nicer than that is "implementation defined behavior." This is a less common phrasing. Undefined behavior and unspecified behavior are common phrasings. Implementation defined behavior is less common. As the C++ spec uses it, implementation defined behavior is behavior which is unspecified by the spec, but any given implementation is expected to provide a definition that you can look up. If my above f(0) example was "implementation defined" rather than "unspecified," the result would be the same except that I would be obliged to provide a document that states what value I chose for f(0). In some cases that is easy. In other cases that's hard.

The final level of definition is to actually specify what happens when a particular assumption is violated. In your example case, this may be hard to do, because there's not many things your algorithm can do if s->str is not a valid string which don't become undefined. However, in many cases you can pick a behavior, and then constrain yourself to it.

So what should you do? The answer depends on how the program will be used. But its worth recognizing that these classes of behavior are generally transitive. If I call a function with undefined behavior, the result of my function is undefined, and so on and so forth all the way up to the program -- we say the program has undefined behavior. Likewise, calling a function with an unspecified value typically results in my result being unspecified as well.

So what behavior is acceptable for a program? If this program is just trying to get a grade in a computer science course, even undefined behavior might be acceptable. But if you're writing an industrial application, undefined behavior can be very bad. Undefined behavior in a robot controller can result in someone being killed. So in an industrial setting, those behaviors aren't accepted. This means that anyone using your function must take the time to prove that your function will not cause undefined behavior when called with their arguments.

This can be expensive. As a result, one may wish to redefine the assumptions. I have written code where init_str not only initializes the string, but creates a note in my own data structures that that particular block of memory was initialized with init_str. do_something then checks to make sure that data was actually allocated before accessing the data in the string. If the check fails, it does something specified or unspecified (such as just returning 0)

In some industries, unspecified behavior is not acceptable. In air traffic control, many applications must be deterministic. All behaviors must be specified.

In all situations, the key to answering your question "should I check" is three fold:

  • Can I check for the condition? If I can't, then obviously I can't do anything about it.
  • Is it affordable to check for the condition? Sometimes I can check, but it takes a ton of CPU time to do it.
  • How does it affect the users of my function?

The last one is really key. If you give me a function with undefined behaviors, I have to grapple with that. I may have to be able to develop a proof that my particular use of the function is always defined. That can be so difficult that I may adjust my algorithm just to make the proof easier.

2
  • A nice rundown of potential difficulty in thoroughly checking arguments, and severity of result of providing invalid arguments. If the language constrained the user more, and accepted more overhead, such thorough checking could be avoided by making many of those invalid states impossible. Dec 3 '21 at 16:26
  • @Deduplicator Very true indeed. There's a fascinating tradeoff between expressiveness and correctness made when constructing a language (or a library written in a language). The managed languages are a great example. Java and C# quite literally cannot have many undefined behaviors that plague C++. There are even some cases where we even sacrifice Turing-Completeness in order to let our tools provide us with hard guarantees on the behavior of the API.
    – Cort Ammon
    Dec 3 '21 at 18:17
1

In general, think of the software built in layers. As data passed between layers, some kind of validation should be performed. Once the data passed validation of a layer, then the same validation shouldn't be performed again by the same layer.

So, if both init_str() and you have do_something1(), do_something2(), etc. and they all live in the same layer, then I wouldn't validate the arguments.

But if they are part of library and they will be called from different layer, i.e. user will call init_str() and then call do_something(), then there's a chance that the data was manipulated by the user, so you should validate.

If your concern is specifically on memory allocation, maybe try library like electric fence which might help. Another way is to have init_str() to also return something like a hash in the string structure, to help detect if the structured had been modified illegally.

0

Minor point: You wrote

s->str=malloc(len*sizeof(char))

As of https://man.openbsd.org/malloc.3

Consider calloc() or the extensions reallocarray() and recallocarray() when there is multiplication in the size argument of malloc() or realloc().

So this could be written

s->str=calloc(len, sizeof s->str[0])

And you might want to check the return value of malloc as well.

4
  • Though by definition sizeof(char) == 1 so that's not a concern in this particular example. You are totally correct though that the standard calloc() should be typically preferred when allocating arrays.
    – amon
    Dec 3 '21 at 11:43
  • @amon If all-bits-zero is a desired starting-point, instead of just needless busywork, yes, calloc() has its place. But that's not a given. Dec 3 '21 at 16:12
  • Rather than indirectly use the type of s->str[0], use it directly: s->str=calloc(len, sizeof s->str[0]). Easier to code right, review and maintain. Dec 4 '21 at 18:46
  • @chux-ReinstateMonica : adjusted, thanks
    – serv-inc
    Dec 6 '21 at 9:41
-1

I agree with the other answer - if you know the software is checking, then you may not want to always check for valid input. However, since you can never expect the software front-end to continue to keep those controls, I would recommend putting reasonable checks in the function to verify input. And what do I mean by reasonable? Basically if it starts to degrade performance unacceptably, then that is not reasonable - but, if it is only programming time that is the issue, then take the time now - you will thank your self later.

You never know what a user will do when they end up interacting with the interface. For example, I had an interface that allowed only numbers to be entered - but the interface would allow the 'e' symbol to be input from the control - and then the server would freak out because it didn't know what that was as it was expecting an integer. Additionally, this can happen with 'i' if your software allows certain numbers to be input.

On a final note - instead of always checking for all the fields, make a couple reference functions you can use in lots of places, like 'ConfirmInteger' or something like that, then send the field into it and perform all the checks you need - then just reuse the modular function anywhere you need to instead of having to retype, and of course update, that function in multiple places.

5
  • 1
    The question was edited to be more clear. Also your example is not really applicable in c. Thanks
    – Harf
    Dec 1 '21 at 23:34
  • @Harf: actually, the answer is language independent. Please remember that a common browser is a typical application written in some form of C (C++, C#...), having a "front end" (what you see on screen) and which can connect to any number of servers. Also, data validation is recommended to be done in any application, regardless of the programming language (even in python, php, java, ruby etc... ). The "garbage in, garbage out" thing is not restricted to C.
    – virolino
    Dec 2 '21 at 11:08
  • @ThomasWest: your answer is good, but not entirely accurate. In C, a function like "ConfirmInteger()" makes no real sense - any pointer can be converted to any type, by the design of the language. However, a function like "CheckRangeOfCarSpeed()" makes a lot more sense - in the most of the usual applications, a speed in excess of about 500kmph is to be ignored - and a failure reported. The same with "CheckIPv4Range()". An IPv4 with integers higher than 255 is not a valid IPv4.
    – virolino
    Dec 2 '21 at 11:21
  • @virolino - your examples are something that could be made modular, yes. Maybe this is my lack of understanding of C, but if I send a function the word 'no', and it is expecitng a number, wouldn't that produce an error due to failed conversion? Also, if i provided the function a negative number, that should also provide an error, yes? If you had a 'CheckForRealisticSpeed' function that you gave it the number first, and it returned a 'yes' or 'no' boolean to allow it to continue, is basically what I meant by validating the data. Dec 4 '21 at 0:10
  • @ThomasWest: it becomes more and more clear that you need more understanding of "C". In C, "n" is a number, "o" is another number, and taken together, "no" is yet another number. It is about ASCII codes, data representation, pointers, strings, all spiced with some math... :)
    – virolino
    Dec 7 '21 at 21:41
-2

Should I assume data passed to my function is accurate?

It depends... If you are writing some C code for a critical device (e.g. neurosurgical robot; then you have to follow standards and use tools like Frama-C) it is different than if you are coding a game (e.g. using libSDL

A possibility is to use so called magic numbers.

1
  • Wow! You know things, and it is still useless. A basic reading of the Wikipedia link you posted tells very clearly that "magic numbers" are an "anti-pattern", which is "is usually ineffective and risks being highly counterproductive". Why did you post this answer which is so wrong? Considering your reputation, you should know better...
    – virolino
    Dec 7 '21 at 21:48

Not the answer you're looking for? Browse other questions tagged or ask your own question.