How to use assert corectly in this function?

Question

I have the following function and I am using assert in a couple of places. I want to know where I am using assert in a wrong way and why. The first one is wrong because we can't use assert on an user input. The second one: can we use assert to check if malloc succeeded? The rest I still can't figure out why. Can you give any help? I would like a short explanation of why assert is good or bad in the given places.

#include <stdio.h>
#include <assert.h>
#include <string.h>
#include <malloc.h>

#define num 10
int hh(char *);

Answer 1

I want this answer to provide an alternative view on your assertions, by trying hard to see a meaningful use of the assertion examples. I otherwise agree with the answer(s) which condemns most of them.

Assertions are used to verify assumptions, i.e. to assert that they are true.
Think of an assertion as

• a method of self-documentation, stating
  "In the following code I blindly assume that the following expression is always true. If that is not the case, something outside of my control is very wrong and this code cannot do what it is supposed to do."
  This self documentation always works, wether the assertions are active or not.
• a technical implementation, which, if active, will verify that the expression is true at runtime, or in case of failure will ring an alarm bell;
  i.e. it will use some appropriate method to get the attention of somebody
  (this method of getting attention can vary greatly; on deeply embedded platforms, the method can get very interesting...)
  Note that assertions usually are implemented in a way that can be deactivated. This serves the purpose of removing their consumption of time, consumption of code size and potential generation of confusing output. Deactivating them is therefor sometimes done before delivering the software, at the end of development/testing. The method of getting attention might not only be unpleasant, it can actually be dangerous. Think of software in a plane, hitting a failed assumption in the air... Horrible and definitly a mistake in the software somewhere. However, in the air, you do not want useful debugging information being conventiently displayed somewhere. You simply want the plane to suck it up and somehow return to normal operation as fast as possible.
  Therefore, the technical implementation part is only useful for problems which occur systematically, i.e. it can be assumed that what can occur in normal use will also occur in the test/development situation. This is the case for most kinds of being misued against specification. This is not the case for transient or rare problems, i.e. problems which occur only in unpredictable situations.

Looking at the assertion examples individually:

1. Blindly assuming that the number of input arguments is 2 seems somewhat daring.
   But if the environment of your program is extremely tightly defined, this is imaginable. Your program might be part of a piping setup or only be called from a makefile. An assertion on the number of input arguments would reflect the larger scope of the architecture and would support error detection in a system which is maintained by a multitude of developers.
   That purpose would however be better served by checking in program code and giving a helpful error message before terminating unsuccessfully.
2. Blindly assuming that malloc does not return zero is a widely spread mistake. Doing that explicity with an assertion, which will at least detect and signal the problem, is actually an improvement on just using malloc without checking. A failure of malloc, caused by system memory being exhausted in an unusual situation, is an example of a problem which cannot usefully be detected with an assertion, i.e. not "systematically", not "misuse against spec".
   Again, checking in program code, with a suitable error message and unsuccessful termiantion, would be better.
3. Blindly assuming a maximum input lenght is daring. What should keep a human from heeding this rule? Especially if there is no output forbidding longer input.
   Like 1, this is imaginable in a tightly defined larger architecture, where humans do not get in the way.
   Again, checking in code with error message would be better, even for a make tool being the user.
4. I agree with John, I simply do not see how this assertion can fail.
   Assertions should decribe assumptions which can actually fail by C syntax and semantic. By contrast, I have seen assert(false) more often than I thought possible. That makes at least the sense of "I blindly assume that runtime execution will never ever end up here." but is otherwise very strange and requires an extensive (and usually missing) comment explanation.
5. Another "humans stay out" assumption, which might make sense in context of make.
   Better checked and error-reported at runtime.
6. This is a typical assertion at the start of a function which uses a pointer and relies on a "promise" that it will only be called with non-NULL pointers. So somewhere in the specification, there should be some "This function can only be called with non-NULL pointers.".
   Checking this at runtime with an error message and unsuccessful termination is still sensible, in order to also be robust against NULL pointers when assertions are deactivated. This might however be considered a waste of time and code size; the more so the more expensive both is in comparison to the confidence in code quality, specification honoring and testing depth.

Answer 2

You define what is right and wrong, so it is up to you, but as a general rule you don't want your program logic in asserts (so checking the length of an input string is a poor use).

Just remember: assert is only active in debug mode, and so if you rely on it for error checking (like almost all of your examples), you will have weird things happening in release mode.

i.e. assert is normally a define like:

/* assert() only does something if NDEBUG is NOT defined */
#ifdef NDEBUG
#else
#define assert(x) _assert(x)
#endif

See Where does the -DNDEBUG normally come from? for more info on NDEBUG

You definitely don't want to change the flow using asserts e.g.

assert(some_function()); /* some_function only called if NDEBUG is not defined */

Using assert to check the return of malloc: If NDEBUG is not defined the check is not done, so in theory you can go off and access a NULL pointer. I'd say it is NOT safe. See Handling error checking with assert more discussion.

Lets look at your asserts one by one through the simple "only in debug" filter:

assert(argc==2);
/*
 * If NDEBUG is defined and argc == 1 you get through,
 * so anything that uses argc[1] will give undefined behavior.
 * = BAD USE
 */
 char *s = malloc(N);
 /* 2 */
 assert(s!=NULL);
 scanf("%s", s
 /*
  * If NDEBUG is defined and malloc fails you keep going and read into 
  * NULL = undefined behaviour = BAD
  */

 assert(strlen(s)<N);
 /*
  * If NDEBUG is defined keeps going even in strlen >= N.
  * How bad that is depends on the code - if you are checking the size
  * before putting it in a buffer, then it's bad.
  * To me it is logic in the assert and so I say BAD
  */
 /* 4 */
 assert(!*(s+strlen(s)));
 /*
  * First of that's just silly code.
  * If that comes to my code review you'll be removing or rewriting
  * it - strlen relies on the NUL, so how can it fail?
  * It is logic in the assert - If NDEBUG is set you'll keep going
  * even if the condition is not met. How bad that is?
  */
 /* 5 */
 assert(atol(s));
 /*
  * Whoa. If NDEBUG is set and s = "Hello" we still keep going...
  * What is atol("Hello") ?
  */
 printf("%ld\n", 100000000/atol(s));
 free(s);
 return 0;
}
int f(char *s)
{
 /* 6 */
 assert(s!=NULL);
 /*
  * Again if NDEBUG is defined the check disappears and so if s = NULL 
  * then you dereference a NULL pointer which is undefined behavior
  */
 return !*s;