[squid-dev] [PATCH] avoid clobbering errno in debugs()

[Kinkie]

On Sun, Jul 12, 2015 at 2:34 AM, Amos Jeffries <squid3 at treenet.co.nz> wrote:

> On 12/07/2015 5:42 a.m., Kinkie wrote:
> > Hi all,
> >   this patch saves and restores errno around calls to strftime in
> > debugLogTime, preventing harmless errors in these calls from clobbering
> > errno, thus messing up xstrerror() calls within debugs().
> >
>
> 1) xerrno is a local variable in lots of places in the code. This patch
> introduces shadowing errors. Please call it debug_xerrno.
>

Ok, no problem.


> 2) The snprintf() call directly after restoring errno in the top chunk
> may alter the original errno in rare cases as well.
>

Are you sure? The snprintf manual page doesn't mention that.


> Which is a good example of why we simply can't rely on errno being
> correct for any length of time in C++. Things such as errno changes by
> operator<< implementations, print() methods, implicit calls to copy
> constructors/destructors, and exlicit function/method calls are also
> possible in ways we are unlikely to be predict.
>
>
> We are still going to have to do a global removal of xstrerror() anyway.
>

I do not object on the technical merit.
At the same time, there are too many to do it incrementally, and we already
failed to reach a consensus on flag-day commit for HERE removal. Sure,
maybe xstrerror()+HERE removal will reach a mass critical enough to support
the flag-day-commit camp, but I don't see that happening, and we already
have quite a but of rework due to HERE being left in changed code. We'd end
up for years with a half-done migration.


> IMO, we should move xstrerror() to Debugs.h and make it use the
> debug_xerrno local variable instead of errno.
>

Maybe we could change debugLogTime to return a std::string instead of a
const char *, and format it ourselves using a stringbuf instead of using
strftime. Or we could maybe do something like this:

int _squid_errno;
const char *xstrerror() {
  return xstrerr(_squid_errno);
}
#define debugs(SECTION, LEVEL, CONTENT) \
   do { \
        if ((Debug::level = (LEVEL)) <= Debug::Levels[SECTION]) { \
            Debug::sectionLevel = Debug::Levels[SECTION]; \
            _squid_errno = errno;
            std::ostream &_dbo=Debug::getDebugOut(); \
            if (Debug::level > DBG_IMPORTANT) \
                _dbo << SkipBuildPrefix(__FILE__)<<"("<<__LINE__<<")
"<<__FUNCTION__<<": "; \
            _dbo << CONTENT; \
            Debug::finishDebug(); \
        } \
   } while (/*CONSTCOND*/ 0)

This would make xstrerror only work within debugs(); maybe we could add a
guard against it by setting _squid_errno to some invalid value and checking
it in xstrerror. Yes, it's ugly.

But until we reach a consensus, I'd like authorization to move forward with
the revised patch I'm attaching. It's far from ideal, but it's a step.

-- 
    Francesco
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150712/43030d2b/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: debug-noclobber-errno-v2.patch
Type: text/x-patch
Size: 488 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20150712/43030d2b/attachment-0001.bin>