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

[Amos Jeffries]

On 12/07/2015 5:45 p.m., Kinkie wrote:
> 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);
> }

Since this is in a header the function needs to be explicitly 'inline'.
The variable needs explict extern with debug.cc safe initialization, etc.

Far simpler and nicer to leave it as a macro:
 #define xstrerror() xstrerr(_sauid_errno)

where _squid_errno as a local to the if-statement scope below.

> #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.

Yes, which is why I requested xstrerror() move to Debug.h as part of the
change. The code not using debugs() can and should use xstrerr(foo) instead.

> 
> 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.
> 

IIRC we already have agreement on removal of xstrerror() globally. Its
not as large as HERE removal and will fix many hidden bugs.

Moving xstrerror() to a debugs-internal mechanism will actually reduce
the size of that change. Which is why I'm continuing with this thread of
discussion at all.

Amos