Hi Ulf,
The historic reason why this happened he way it happened was due to our
negotiations with EPEL on what could and could not change in public
headers. We finally sorted that out but that was way after that code was
in place. As for the other code snippet, I will take a look.
By the way, we always appreciate code reviews and people pointing out
prolematic code. Unfortunately, sometimes odd code does get into any large
project for various strange reasons. So, thank you for pointing that out.
Andy
On Tue, 21 Oct 2014, Ulf Tigerstedt wrote:
> On 19/10/14 01:12, Andrew Hanushevsky wrote:
>> Hi Ulf,
>>
>> OK, after remapping the addresses in your stack trace to get the exact
>> code path, I did indeed discover where the stack corruption came from.
>> It is not caused by any user data so it cannot be used to compromise a
>> server. It's simply that a too small buffer was passed to an internal
>> function. Additionally, the problem would only manifest itself at log
>> file rotation time (usually midnight). This is now fixed in git head.
>>
>> Since you have compiled 4.0.3 from source, you can apply the following
>> source fix:
>>
>> XrdSysLogger.cc:418: char tbuff[24];
>> should be changed to
>> XrdSysLogger.cc:418: char tbuff[32];
>>
>> I will check with Lukasz whether we will back port this. Thank you for
>> bringing this to our attention.
>
> Finally wake enough to check the code: Wouldn't it be sensible to use a
> constant #defined for this? Having magic numbers spread around the code is
> just begging for disaster.
>
> Grepping for "buff" in the code turns up more problems:
>
> XrdSysError.cc contains this gem:
> char ebuff[16], etbuff[80], *etxt = 0;
>
> if (!(etxt = ec2text(ecode)))
> {snprintf(ebuff, sizeof(ebuff), "reason unknown (%d)", ecode);
> etxt = ebuff;
> } else if (isupper(static_cast<int>(*etxt)))
> {strlcpy(etbuff, etxt, sizeof(etbuff));
> *etbuff =
> static_cast<char>(tolower(static_cast<int>(*etxt)));
> etxt = etbuff;
> }
>
> that snprintf won't ever create anything else than "reason unknown ", so why
> do it the complicated way at all?
>
> const char ebuff[]="reason unknown";
> would do just as well for this, just as useful.
>
> We removed the logrotate until we get a new release..
>
>
>
> --
> Ulf Tigerstedt || CSC Oy || NDGF
> Computing Environments group
> (NGI_FI and NGI_NDGF) deputy manager
> GSM +358503818558 || +35894572279
> Keilaranta 14 || Espoo || Finland
>
########################################################################
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-L list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-L&A=1
