[ROOT-6239] TClass::EscapeChars seg fault Created: 17/Apr/14  Updated: 21/Apr/14  Resolved: 17/Apr/14

Status: Closed
Project: ROOT
Component/s: Core Libraries
Affects Version/s: 5.34/00
Fix Version/s: None

Type: Bug Priority: High
Reporter: Daniel Geerts Assignee: Philippe Canal
Resolution: Fixed Votes: 0
Labels: None
Environment:

all


Development:

 Description   

char *TClass::EscapeChars(const char *text) const allocates a 128-byte static buffer for its output. It checks if the input text isn't larger than 127 chars (good), but after escaping, the output written to the buffer may be larger, leading to memory corruption.

The interpreter doesn't (always) crash: silent memory corruption. G++-compiled programs usually get very upset (seg fault). Code to reproduce:

#include "TClass.h"

#include <iostream>
using namespace std;

int main()
{
TClass a;
cout << a.EscapeChars("[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[") << endl;
return 0;
}



 Comments   
Comment by Philippe Canal [ 17/Apr/14 ]

Hi,

The problems has been fixed in the v5.34 patch branch and the trunk.

Thanks for your report.
Cheers,
Philippe.

Comment by Daniel Geerts [ 18/Apr/14 ]

Hi Philippe,

I think there's a small issue with your fix. If I have a string with exactly 255 chars (excl NULL), with the last character being one that needs to be escaped, icur will be increased twice in the loop, and the closing NULL will be written outside of the array. Also, I think the code would be a bit more understandable if maxsize is used in the array declaration, and maxsize should really be declared 'const' (and not static).

As you probably already figured out, all three issues can be solved by changing the first two lines to:
const UInt_t maxsize = 255;
static char name[maxsize+2]; //One extra if last char needs to be escaped

Sorry for being so nitpicky.

Comment by Philippe Canal [ 18/Apr/14 ]

Hi Daniel,

I agree with you. I updated the code accordingly.

Thanks,
Philippe.

Comment by Daniel Geerts [ 20/Apr/14 ]

Sorry, didn't know where to report this, but currently in the ROOT patch changelog, the wrong bug-number is being referenced for this ticket:
Prevent out-of-bounds access in TClass::EscapeChars. (ROOT-5864.).

That should be ROOT-6239.

Comment by Daniel Geerts [ 20/Apr/14 ]

Wait, another entry is referencing this ticket:
Fix memory leak in TKeyXML (ROOT-6239.).

I guess a copy-paste mistake happened?

Comment by Philippe Canal [ 20/Apr/14 ]

Hi Daniel,

I am a little puzzled as I do not see those. For example I see:

Fix out-of-bounds problem in TClass::EscapeChars
    
    Increase the maximum size to 255 and make sure that the new, expanded string fix in the fixed length buffer.
    This fixes ROOT-6239

Cheers,
Philippe

Comment by Daniel Geerts [ 21/Apr/14 ]

Sorry, I should've been more specific. I'm looking here: http://root.cern.ch/drupal/content/root-version-v5-34-00-patch-release-notes

Changes in head of v5-34-00-patches branch

Core
Fix compiler warning in TArray (ROOT-5864.).
Prevent out-of-bounds access in TClass::EscapeChars. (ROOT-5864.).
I/O
Fix memory leak in TKeyXML (ROOT-6239.).
Prevent random behavior in case of corrupted file (fNbytesFree)(ROOT-6240.).
Allow TBranch::DropBaskets to drop the write basket if it is empty.

Comment by Philippe Canal [ 21/Apr/14 ]

Hi Daniel,

Thanks for reporting this issue. The release notes have been fixed.

Cheers,
Philippe.

Generated at Sat Sep 21 07:29:21 CEST 2019 using Jira 7.13.1#713001-sha1:5e06076c2d215a6f699b7e5c90ab2fae7ba5a1ce.