One of the warnings that caused a tad annoyance recently was my sloppy use of asprintf. This is a lovely function which does a printf but allocates memory as needed to do it. It avoids one of the big issues with sprintf over running space and why you must always use snprinft and not sprintf. Obviously it needs tidying (free).
My sloppiness was simple, I was not checking the return value. It is always important to check return values, though, to be fair, with memory limits so high these days the time a malloc fails is pretty unlikely.
So, the simple answer is wrap it in an assert. If it fails, that is fatal, end of story. Easy to track and debug if it happens and a lot easier than a random seg fault or uninitialised pointer being used.
But that makes the code messy, so bingo, lets make a safeasprintf that can assume it worked.
How to do it? Why not something simple like :-
#define safeasprintf(...) assert(asprintf(__VA_ARGS__)>=0)
That is simple. It takes whatever I put in the safeasprintf and does an asprintf with it checking there is no error (-ve) return value, and aborts if there is. A bit of a global edit s/asprintf/safeasprintf/g and Bob's some relative. No warnings and safer code.
Except... There is, it seems, a specific build of linux, gcc, cpp, whatever, that does not work. Works all over the place, but on a couple of machines it does not. It does not generate an error but treats the whole macro as nothing. The clue was warnings that variables defined and then used in a safeasprintf were "defined but not used". The bigger clue was the code just not working in all sorts of places. Every asprintf had not in fact printed a thing! I was almost impressed my code did not crash horribly as a result!
I have no idea what cpp did. If it did not understand __VA_ARGS__ then surely that would have left gcc with an error? How could it fail in such a special way? Even the asserts did not barf.
Well, what can I say, lesson learned.
int
safeasprintf (char **strp, const char *fmt, ...)
{ // Do asprintf but abort if error
va_list ap;
va_start (ap, fmt);
int ret = vasprintf (strp, fmt, ap);
va_end (ap);
if (ret < 0)
errx (1, "Bad asprintf");
return ret;
}
P.S. Looks a lot like it is the mis-use of assert that is the issue. It does nothing. I understood it did not check and abort but in spite of decades of C coding I did not know it actually did not do the expression within the assert either. You learn something new every day.
assert() is designed to compile away to nothing in a release build, where NDEBUG is defined.
ReplyDeleteIt's not the right tool to verify run-time assumptions.
Fair enough.
DeleteThe reason for evaluating to nothing when NDEBUG is defined is because the arguments to assert() may contain expensive computations to be verified by the assertion, and you don't want to waste time doing those computations when assertions are disabled.
ReplyDeleteThe assumption was that nobody would be so silly as to put stuff with side-effects into an assert and then expect them to persist even when you wanted the assert to do nothing. The resulting decades of bugs have taught us that this assumption was in error :)
Indeed! I am checking any other places I have mis-used assert now :-)
DeleteThe NDEBUG "feature" (more like madness) is why I've never used assert() in 30 years of C programming. Until now that is, my new place mandates it for error handling. I have been unable to persuade them this is a bad idea (they've been doing it for 15 years and are in denial there's a problem with it).
DeleteGlad I was not being totally daft then :-) I think a custom assert code of my own with stack trace and so on it probably better.
DeleteOn the assumption that you're using either GCC or Clang, did you annotate the function declaration?
ReplyDelete__attribute__ ((format (printf, 2, 3)))
__attribute__ ((nonnull (1)))
To be honest I am not up to speed on all of the cxx attributes and was thinking it would be handy to define some of these things, yes. So thanks for the tip.
Deletegcc
DeleteI tend to use something similar for any call which shouldn't fail but theoretically could, to make sure I do handle that situation sanely - I remembered about assert() getting compiled out on non-debug builds, though.
ReplyDeleteI'm not convinced of the logic of that exclusion these days (aren't most "unlikely" scenarios like that actually *more* likely to need to be caught in public releases of software than in internal test builds?!) - an "always_assert_even_in_release_mode" does seem useful too. In my recent Go coding, I've tended to include a simple "checkdie" function, which checks for error values and dies with an appropriate message on failure. So, "dostuff()" which might return an error becomes a simple "checkdie(dostuff())" which quits if that operation fails - quite sensible for batch data conversion, which is where I'm using Go at the moment.
The discussion of this on StackOverflow also included someone pointing out changing lots of settings between the debug internal test build and the public release seems unwise, better to test the version you'll be shipping and vice versa as far as possible.
Indeed, I am even thinking I need an assert type function that not only reports and exist but also does stack trace so I know how it got there. This is, indeed, important in live code as it quickly identifies the problems that need addressing urgently as they are in a live system!
DeleteIf this is server-side stuff, you really ought to look at something more modern than 'C' - these sort of problems are just dealt with so much better in modern languages / frameworks.
DeleteMany options I know - I like C though :-)
Delete