[core/zip+lzma] Properly account for header size #14523

hahnjo · 2024-02-02T09:03:00Z

The compression algorithms only see the buffers without the header, so the sizes have to be adjusted accordingly.

phsft-bot · 2024-02-02T09:03:09Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

hahnjo · 2024-02-02T09:06:40Z

IMHO this is quite bad - to how many ROOT versions should we backport this? This problem is basically hiding there all the way since commit 4b54256 in 2011!

phsft-bot · 2024-02-02T10:41:48Z

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

And 5 more

github-actions · 2024-02-02T11:42:47Z

Test Results

10 files 10 suites 1d 22h 34m 10s ⏱️
2 497 tests 2 495 ✅ 0 💤 2 ❌
23 869 runs 23 593 ✅ 272 💤 4 ❌

For more details on these failures, see this check.

Results for commit 45e09b2.

♻️ This comment has been updated with latest results.

phsft-bot · 2024-02-02T12:46:45Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

hahnjo · 2024-02-02T12:49:28Z

So @jblomer naively asked "what about ZLIB", and it turns out to be equally wrong... I also added a test that at least catches the compression side of things. For the decompression, it's a bit harder because it's not clear how to check if the library read more bytes than it should have (without it running into errors because of decompression errors).

hahnjo · 2024-02-02T15:11:51Z

After more investigation, it seems that all existing code paths in TKey.cxx, TBufferXML.cxx, TMessage.cxx, and TBasket.cxx allocate a buffer that is slightly larger, so it's probably not an as critical problem for the non-RNTuple case...

core/zip/test/ZipTest.cxx

core/zip/src/RZip.cxx

phsft-bot · 2024-02-03T09:31:31Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

pcanal

(pending review)

hahnjo · 2024-02-05T16:15:51Z

To reiterate on why we "only" need to fix gzip and lzma: The other compression algorithms already do this,

root/core/lz4/src/ZipLZ4.cxx

Lines 54 to 58 in e8545f7

    
           if (cxlevel >= 4) { 
        
              returnStatus = LZ4_compress_HC(src, &tgt[kHeaderSize], *srcsize, *tgtsize - kHeaderSize, cxlevel); 
        
           } else { 
        
              returnStatus = LZ4_compress_default(src, &tgt[kHeaderSize], *srcsize, *tgtsize - kHeaderSize); 
        
           }

root/core/zip/src/RZip.cxx

Lines 145 to 148 in e8545f7

    
           state.out_buf     = tgt; 
        
           state.out_size    = (unsigned) (*tgtsize); 
        
           state.out_offset  = HDRSIZE; 
        
           state.R__window_size = 0L;

(that's the very original code with the old compression algorithm; it uses an offset which is correct by construction)

root/core/zstd/src/ZipZSTD.cxx

Lines 32 to 35 in e8545f7

    
           size_t retval = ZSTD_compressCCtx(fCtx.get(), 
        
                                               &tgt[kHeaderSize], static_cast<size_t>(*tgtsize - kHeaderSize), 
        
                                               src, static_cast<size_t>(*srcsize), 
        
                                               2*cxlevel);

pcanal · 2024-02-06T00:11:34Z

Note that the problem appears (and/or is uncovered) 'recently' and was "introduced" by e052b58: [ntuple] RPageSinkBuf: Always seal before CommitCluster (prior to this commit valgrind is silent).

hahnjo · 2024-02-06T07:49:14Z

Somewhat, that commit made it more likely for regular users to run into the problem: Essentially, the commit moved code around to always seal pages in CommitPage. Before it was only done when implicit MT was enabled, and the original reproducer code fails in ROOT 6.30 with a preceding call to ROOT::EnableImplicitMT(). In my understanding, that's also what CMSSW does, so for them it doesn't make a difference. (In fact, the problematic pattern of exactly allocating as many bytes as the uncompressed page holds can be traced back to commits 1ea8447 and 88bd1f0 at the very beginning of RPageSinkBuf's history)

hahnjo · 2024-02-07T16:34:11Z

ping @pcanal it would be really good to have the fix in, my understanding is that it blocks CMS RNTuple work...

core/zip/test/ZipTest.cxx

R__unzipZLIB is already properly subtracting it from srcsize.

lzma_code must only see the buffers without the header, so the sizes have to be adjusted accordingly. Fixes root-project#14508

In practice, the target size is greater or equal the source size in most cases for ROOT, but add this additional correct check to fuzz the inputs in the next commit.

This would have found any of the previous three commits.

phsft-bot · 2024-02-07T20:07:43Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

pcanal · 2024-02-07T20:22:08Z

To reiterate on why we "only" need to fix gzip and lzma: The other compression algorithms already do this,

Indeed. The diffs was made less obvious because:
ZLIB decompression is already doing the right thing.
ZLIB and LZMA use a struct to pass the configuration rather than function argument so the code pattern is slight different.

it seems that all existing code paths in TKey.cxx, TBufferXML.cxx, TMessage.cxx, and TBasket.cxx allocate a buffer that is slightly larger, so it's probably not an as critical problem

Right, the allocations is done:

      Int_t buflen = TMath::Max(512,fKeylen + fObjlen + 9*nbuffers + 28); //add 28 bytes in case object is placed in a deleted gap

and used via

          char *bufcur = &fBuffer[fKeylen];

so the only extra is 9*nbuffers + 28 which reduces the risk of writing the end since the size is larger than fObjlen + kHeaderSize but that leaves 2 additional question:

why are those added?
why doesn't RNTuple need it?

01bb696 hints that the compression engine were seen as writing past the end ... it is plausible since the prior delta was ``9*nbuffers + 8withnbuffers==0` is common case. (in hindsight, this commit was not investigated long enough and needed a test).

The 9*nbuffers is meant to be for the keys and is now inaccurate (most algorithms have a 9 bytes header but for lz4 we have seemingly 73. This part is missing from the RNTuple usage. The consequences is that on data set that is not compressible TTree might use a bit more space (header + barely compressed size) vs RNTuple (uncompressed size which might be less than header + barely compressed size).

This of course assume that the compression algorithm strictly respect the limit given (it would be a serious security risk if not).

The 8 is commented as "8 bytes in case object is placed in a deleted gap" (the 20 was seemingly added to work-around the bug fixed here) ~~and is not clear to me (the 'delete gap' is most likely talking about a space 'freed' inside a ROOT file.~~

pcanal

LGTM. Thanks. This patch needs to be backported to as many older releases as possible as it can lead to a memory over-write even in the case of TTree (the compression is being given a memory area smaller than it is and unless the compression algorithm stops before it has over-inflated the object by 28+9 bytes, it might still happens)

A a side note, the extra size given by TKey and TBaskets probably should be removed (delta understanding why there was a +8 "in case object is placed in a deleted gap".

pcanal · 2024-02-07T21:07:54Z

(delta understanding why there was a +8 "in case object is placed in a deleted gap".

I am now guessing that this was a micro optimization to better manage the memory. We should also consider to remove it.

hahnjo · 2024-02-08T08:19:58Z

01bb696 hints that the compression engine were seen as writing past the end ... it is plausible since the prior delta was 9*nbuffers + 8 with nbuffers==0 is common case. (in hindsight, this commit was not investigated long enough and needed a test).

I think nbuffers >= 1 in all cases, so we should always have 9 additional bytes beyond what we tell R__zipMultipleAlgorithm.

This of course assume that the compression algorithm strictly respect the limit given (it would be a serious security risk if not).

Yes, we have to operate under that assumption.

This patch needs to be backported to as many older releases as possible as it can lead to a memory over-write even in the case of TTree (the compression is being given a memory area smaller than it is and unless the compression algorithm stops before it has over-inflated the object by 28+9 bytes, it might still happens)

Yes, I think the compression algorithms stop at the buffer sizes we give them. Unless I'm missing something, this means only RNTuple was affected by this and TTree was fine because of the slightly larger buffers? For now, I've opened backports for 6.30 (#14624), 6.28 (#14625), and 6.26 (#14626). If we find that TTree is also affected, we can (and have to) open more backports.

A a side note, the extra size given by TKey and TBaskets probably should be removed (delta understanding why there was a +8 "in case object is placed in a deleted gap".

Ok, we can try (in master). We have to be careful though, I don't want to introduce more memory errors for writing TTrees...

hahnjo added priority:critical in:Core Libraries labels Feb 2, 2024

hahnjo requested review from jblomer and pcanal February 2, 2024 09:03

hahnjo self-assigned this Feb 2, 2024

hahnjo requested a review from dpiparo as a code owner February 2, 2024 09:03

hahnjo force-pushed the core-zip branch from 1eda9e4 to f21e4b3 Compare February 2, 2024 12:46

hahnjo requested a review from bellenot as a code owner February 2, 2024 12:46

hahnjo changed the title ~~[core/lzma] Properly account for kHeaderSize~~ [core/zip+lzma] Properly account for header size Feb 2, 2024

hahnjo removed the priority:critical label Feb 2, 2024

pcanal reviewed Feb 2, 2024

View reviewed changes

core/zip/test/ZipTest.cxx Show resolved Hide resolved

pcanal reviewed Feb 2, 2024

View reviewed changes

core/zip/src/RZip.cxx Outdated Show resolved Hide resolved

hahnjo force-pushed the core-zip branch from f21e4b3 to 45e09b2 Compare February 3, 2024 09:31

jblomer approved these changes Feb 5, 2024

View reviewed changes

pcanal requested changes Feb 5, 2024

View reviewed changes

pcanal reviewed Feb 7, 2024

View reviewed changes

core/zip/test/ZipTest.cxx Outdated Show resolved Hide resolved

hahnjo added 2 commits February 7, 2024 21:02

[core/zip] Properly account for HDRSIZE in R__zipZLIB

1df145a

R__unzipZLIB is already properly subtracting it from srcsize.

[core/lzma] Properly account for kHeaderSize

723e162

lzma_code must only see the buffers without the header, so the sizes have to be adjusted accordingly. Fixes root-project#14508

hahnjo added 2 commits February 7, 2024 21:02

[core/zip] Validate target size before compression

fb736e2

In practice, the target size is greater or equal the source size in most cases for ROOT, but add this additional correct check to fuzz the inputs in the next commit.

[core/zip] Add test for compression buffer sizes

173ca7b

This would have found any of the previous three commits.

hahnjo force-pushed the core-zip branch from 45e09b2 to 173ca7b Compare February 7, 2024 20:07

pcanal approved these changes Feb 7, 2024

View reviewed changes

hahnjo merged commit 73d8c3d into root-project:master Feb 8, 2024
13 of 15 checks passed

hahnjo deleted the core-zip branch February 8, 2024 07:31

This was referenced Feb 8, 2024

[core/zip+lzma] Properly account for header size [v6.30] #14624

Merged

[core/zip+lzma] Properly account for header size [v6.28] #14625

Merged

[core/zip+lzma] Properly account for header size [v6.26] #14626

Merged

pcanal mentioned this pull request Feb 10, 2024

Reduce memory allocation size for compression buffer #14651

Open

hahnjo mentioned this pull request Mar 25, 2024

[io] Validate ZSTD target/source sizes and compr level #15038

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core/zip+lzma] Properly account for header size #14523

[core/zip+lzma] Properly account for header size #14523

hahnjo commented Feb 2, 2024 •

edited

Loading

phsft-bot commented Feb 2, 2024

hahnjo commented Feb 2, 2024

phsft-bot commented Feb 2, 2024

github-actions bot commented Feb 2, 2024 •

edited

Loading

phsft-bot commented Feb 2, 2024

hahnjo commented Feb 2, 2024 •

edited

Loading

hahnjo commented Feb 2, 2024

phsft-bot commented Feb 3, 2024

pcanal left a comment

hahnjo commented Feb 5, 2024

pcanal commented Feb 6, 2024 •

edited

Loading

hahnjo commented Feb 6, 2024

hahnjo commented Feb 7, 2024

phsft-bot commented Feb 7, 2024

pcanal commented Feb 7, 2024 •

edited

Loading

pcanal left a comment

pcanal commented Feb 7, 2024

hahnjo commented Feb 8, 2024

[core/zip+lzma] Properly account for header size #14523

[core/zip+lzma] Properly account for header size #14523

Conversation

hahnjo commented Feb 2, 2024 • edited Loading

phsft-bot commented Feb 2, 2024

hahnjo commented Feb 2, 2024

phsft-bot commented Feb 2, 2024

Failing tests:

github-actions bot commented Feb 2, 2024 • edited Loading

Test Results

phsft-bot commented Feb 2, 2024

hahnjo commented Feb 2, 2024 • edited Loading

hahnjo commented Feb 2, 2024

phsft-bot commented Feb 3, 2024

pcanal left a comment

Choose a reason for hiding this comment

hahnjo commented Feb 5, 2024

pcanal commented Feb 6, 2024 • edited Loading

hahnjo commented Feb 6, 2024

hahnjo commented Feb 7, 2024

phsft-bot commented Feb 7, 2024

pcanal commented Feb 7, 2024 • edited Loading

pcanal left a comment

Choose a reason for hiding this comment

pcanal commented Feb 7, 2024

hahnjo commented Feb 8, 2024

hahnjo commented Feb 2, 2024 •

edited

Loading

github-actions bot commented Feb 2, 2024 •

edited

Loading

hahnjo commented Feb 2, 2024 •

edited

Loading

pcanal commented Feb 6, 2024 •

edited

Loading

pcanal commented Feb 7, 2024 •

edited

Loading