2008.12.17 20:54 "[Tiff] Deleting tags from a directory", by Frank Warmerdam

2009.02.06 20:34 "Re: [Tiff] assertions, and building with DEBUG/NDEBUG", by Phillip Crews

On Thu, 5 Feb 2009, Phillip Crews wrote:

In a library like libtiff, it is inappropriate to abort the calling program when an assertion is triggered, which a failed assert() does on many platforms.

In my code, I use my own macro, which is defined as assert() in debug builds, but in release builds calls an internal logging routine and continues. If a severe problem or crash occurs later because of the bad value, I have information to go on in the log.

It is quite often (most often) not appropriate to proceed with processing once an assert has been encountered. Doing to risks heap corruption, and potential exploits from intentionally malligned files. If any heap or stack corruption has occured, then the process is then in an instable state.

Unfortunately, this is neither how assert() was designed, nor how it is implemented on the platforms I'm familiar with. Of course, assert() should never be used for external data validation. In practice, it's not always easy to analyze how someone else used it.

The key thing is to discriminate between issues which are best reported as errors, warnings, or assertions. Assertions should always stop processing since they should be used to detect logic (i.e. design/implementation) errors. Warnings result in a message to the user, but processing continues. Errors result in a message to the user, and a return to the invoking code with no further processing of the current file.

The default behavior for assert() in production builds (NDEBUG) on most platforms is to not even do the check, much less to log, which is why I defined my own assertion macro and supporting routines. Because of unpredictability in the large number of libraries I use, I do not attempt to build with neither DEBUG nor NDEBUG defined. Some libraries I use specifically state that NDEBUG should be defined for production builds, though I don't have enough in-depth knowledge or free time to analyze them all.

I am careful with my assertions -- I use them for sanity, parameter, handle, scalar size, thread, or constant validations -- verifying assumptions made by the code or even reminders for the future. Examples:

  CS_ASSERT(sizeof(HFILE) == sizeof(long)); /* Following code assumes
handles are 32-bit */
  CS_ASSERT(CS_ISGUITHREAD()); /* Must only be called from user
interface thread */
  CS_ASSERT(open_count < 200); /* Something isn't closing files properly! */
  CS_ASSERT(origin == SEEK_SET || origin == SEEK_END || origin == SEEK_CUR);

I handle buffer checks, range checks, and other things that could result in invalid memory accesses as errors: by returning FALSE, NULL or an error code, by setting a default value, or by raising an exception (in C++ code). Many of these include a CS_ASSERT(0) so I'll see the problem immediately when testing internally.

Anyway, back to what libtiff should do: I don't see my style of logging as a viable option unless it's done with platform and application-specific callbacks, at which point you're pretty much restricted to using TIFFWarning()/TIFFError() unless you're willing to adopt significant ABI changes.

One way would be to define a TASSERTW (warn and continue) for cases where processing can continue, a TASSERTF (fatal) which actually calls TIFFError() and bails using abort() -- this should be used minimally if at all, considering that libtiff is used in large-scale GUI applications and daemons, which need to completely fail as rarely as possible. An additional define -DOLDASSERT could be used to provide the current assert behavior.

#if defined(OLDASSERT)
#define TASSERTW(x) assert(x)
#define TASSERTF(x) assert(x)
#else

#define TASSERTW(x) (void)( (!!(x)) || (TiffAssertW(x, __FILE__, __LINE__, __func__), 0) ) #define TASSERTF(x) (void)( (!!(x)) || (TiffAssertF(x, __FILE__, __LINE__, __func__), 0) )

#endif

"Wrong" assertions of course need to be converted to actual error returns; unfortunately, this is not a trivial task, since each requires analysis, a lot of them are deep, and all callers will need to be examined to be sure they propagate the error correctly. There shouldn't be a lot of them, however.