#109 Invalid Cx86ConvertOutStream object in NLzx::CDecoder::Flush

open
nobody
None
5
2011-02-01
2011-02-01
No

The file (the file is about 5M and cannot be attached directly, please see http://narod.ru/disk/4749634001/crash01022011.chm.html\) was extracted by 7z from a multivolume RAR archive, as a result a section of this file is corrupted.

When extracting the files from this CHM, when the extraction process reaches the corrupted (non-existant) files I experience random crashes in 7z with the following stack trace

NCompress::NLzx::Cx86ConvertOutStream::MakeTranslation () from
/usr/lib/7z.so
#1 0xf4889214 in NCompress::NLzx::Cx86ConvertOutStream::Flush () from
/usr/lib/7z.so
#2 0xf48896b9 in NCompress::NLzx::CDecoder::Flush () from /usr/lib/7z.so
#3 0xf488b530 in NCompress::NLzx::CDecoderFlusher::~CDecoderFlusher
() from /usr/lib/7z.so
#4 0xf488b3c9 in NCompress::NLzx::CDecoder::CodeReal () from
/usr/lib/7z.so
#5 0xf488b42f in NCompress::NLzx::CDecoder::Code () from /usr/lib/7z.so
#6 0xf48285b3 in NArchive::NChm::CHandler::Extract () from /usr/lib/7z.so

The Cx86ConvertOutStream object looks unitialized with random values instead of the member variables. valgrind also keeps complaining about the unitialized values in the object functions. Apparently the Init function wasn\'t called, while the default constructor doesn\'t initialize the values properly.

I\'d suggest to add a constructor to the Cx86ConvertOutStream class which would initialize the fields properly. But more likely the problem is the handling of the corrupted files by the LzxDecoder, which may call Flush on its m_x86ConvertOutStreamSpec object even if it was never used - in such case there would be no need to flush it.

I\'d like to emphasize that this isn\'t a problem of broken archive, this is an issue of the 7z stability and internal consistency.

Discussion

  • Martin Lemke

    Martin Lemke - 2011-02-16

    I suggest the following patch for this issue:

    diff -urN p7zip_9.13/CPP/7zip/Compress/Lzx86Converter.h p7zip_9.13.patch/CPP/7zip/Compress/Lzx86Converter.h
    --- p7zip_9.13/CPP/7zip/Compress/Lzx86Converter.h 2009-02-07 17:06:28.000000000 +0000
    +++ p7zip_9.13.patch/CPP/7zip/Compress/Lzx86Converter.h 2011-02-16 07:40:17.000000000 +0000
    @@ -25,6 +25,13 @@

    void MakeTranslation();
    public:
    + Cx86ConvertOutStream()
    + {
    + m_TranslationMode = 0;
    + m_TranslationSize = 0;
    + m_ProcessedSize = 0;
    + m_Pos = 0;
    + }
    void SetStream(ISequentialOutStream *outStream) { m_Stream = outStream; }
    void ReleaseStream() { m_Stream.Release(); }
    void Init(bool translationMode, UInt32 translationSize)

    In order to reproduce the bug just replace the zeroes in this patch with something random, e.g. 0x7FFFFFF - the crash will occur almost immediately when running the test file. It is clearly seen that Cx86ConvertOutStream object is created, decoding fails so it goes without calling Init so all the object members are still having random initialization values, but then Flush is called.

     
  • Martin Lemke

    Martin Lemke - 2011-02-16

    The same issue also exists in version 9.20 in the main 7z repository.

     
  • Igor Pavlov

    Igor Pavlov - 2011-02-22

    I understand that it's BUG.
    But I can't reproduce crash for Windows version. I see only "Data Error".
    What exact command do you use?
    Do you extract only particular file from that CAB?

     
  • Martin Lemke

    Martin Lemke - 2011-02-22

    The crash randomly occurs when I am using the 7z dll (in fact, an .so on debian linux) from my application. Valgrind reports uninitialized values also when I am trying to extract all the items from this CHM archive using a standalone 7z application (it never crashed though).

    Most likely the memory is initialized differently when I am running a standalone application or a 7z dll in a context of another large application. If it happened that the memory block used by Cx86ConvertOutStream object is initialized with zeroes the crash wouldn't happen, the execution flow will never reach MakeTranslation() or other functions that require the object to be properly initialized. If the memory is filled with a random values the crash occurs at some point.

    If I am not mistaken, BoundsChecker on Windows have a tool that fills all newly allocated objects with a special non-zero pattern, specifically to catch this kind of uninitialized object problems.

    As I mentioned, you can emulate the problem by filling the content of Cx86ConvertOutStream object with some artificial values in the default constructor:

    Cx86ConvertOutStream()
    {
    m_TranslationMode = 0xFFEFEFEF;
    m_TranslationSize = 0xDEDEDEDE;
    m_ProcessedSize = 0x77777777;
    m_Pos = 0x7777FFFF;
    }

    (though I must admit that I didn't try it with the latest Windows code)

    The execution flow in LzxDecoder expect this object to be initialized by calling Init prior being used. However processing this corrupted archive and reaching the broken items the Init is never called - but Flush still does.

     
  • Igor Pavlov

    Igor Pavlov - 2011-02-23

    Please try another way to fix that bug:

    STDMETHODIMP CDecoder::SetOutStreamSize(const UInt64 *outSize)
    {
    if (outSize == NULL)
    return E_FAIL;
    if (!_keepHistory)
    m_x86ConvertOutStreamSpec->Init(false, 0);
    _remainLen = kLenIdNeedInit;
    ....

    Does it fix problems?

     
  • my p7zip

    my p7zip - 2011-02-23

    I tried crash01022011.chm with p7zip 9.13 and p7zip 9.20 (not yet released)
    on Ubuntu 10.10.

    I tried the 32bits and 64bits built and I don't have crashes.

    I tried to run the tests under valgrind and valgrind detected 0 errors ...

    example :
    valgrind 7z x crash01022011.chm
    displays " 0 errors"

     
  • Martin Lemke

    Martin Lemke - 2011-02-24

    The suggested patch in SetOutStreamSize also solves the problem, as well as the earlier patch to Cx86ConvertOutStream.

    The original code causes the following errors in valgrind with gdb attached:

    ==20753== Conditional jump or move depends on uninitialised value(s)
    ==20753== at 0x6860EAB: NCompress::NLzx::Cx86ConvertOutStream::Flush() (Lzx86Converter.cpp:72)
    ==20753== by 0x6862251: NCompress::NLzx::CDecoder::Flush() (LzxDecoder.cpp:33)
    ==20753== by 0x6862796: NCompress::NLzx::CDecoderFlusher::~CDecoderFlusher() (LzxDecoder.cpp:154)
    ==20753== by 0x686215F: NCompress::NLzx::CDecoder::CodeReal(ISequentialInStream*, ISequentialOutStream*, unsigned long long const*, unsigned long long const*, ICompressProgressInfo*) (LzxDecoder.cpp:336)
    ==20753== by 0x68621C5: NCompress::NLzx::CDecoder::Code(ISequentialInStream*, ISequentialOutStream*, unsigned long long const*, unsigned long long const*, ICompressProgressInfo*) (LzxDecoder.cpp:342)
    ==20753== by 0x67F8FAC: NArchive::NChm::CHandler::Extract(unsigned const*, unsigned, int, IArchiveExtractCallback*) (ChmHandler.cpp:689)

    ...

    ==20753== ---- Attach to debugger ? --- [Return/N/n/Y/y/C/c] ---- y
    ==20753== starting debugger with cmd: /usr/bin/gdb -nw /proc/20762/fd/1014 20762

    ...

    0x06860eab in NCompress::NLzx::Cx86ConvertOutStream::Flush (this=0x6289280) at ../../Compress/Lzx86Converter.cpp:72
    72 if (m_TranslationMode)
    (gdb) p *this
    $1 = {
    <ISequentialOutStream> = {
    <IUnknown> = {
    _vptr.IUnknown = 0x68daa08
    }, <No data fields>},
    <CMyUnknownImp> = {
    __m_RefCount = 2
    },
    members of NCompress::NLzx::Cx86ConvertOutStream:
    m_Stream = {
    _p = 0x6319050
    },
    m_ProcessedSize = 348474451,
    m_Pos = 1393832268,
    m_TranslationSize = 1281344817,
    m_TranslationMode = 203,
    m_Buffer = "O\224a\213\177*ó¦\t\205íOx\2040¨ó+c¦îwÜ\030â­+P\211D_\230\aÍÝ\206\215à\003\227\225F\226\234\n\204*­\r Ôüë;B\017êü÷\n\224Aj\220\215B\216\212\037¯/ð¹\225&\225À|ñ\037÷?tÛ.z¢Rï\226Î\fï-r~\205¡\024qUÓè\200\030#\tL0/h5\\PïnÎ\220\226:)ûjV7kÜ\036g|\nü\030ð\216Ë\031-LW¢\týæ²d~·/ÛO\032a\234N\0273È\235në!G\000\206®\034îpur\225½5\203N\0238]\005\213hàæ\212C[Ëô¹×ÝhºU=E\230'4"...
    }

    I must admit that with the 7z application I for some reason no longer can reproduce the valgrind "unitialized value" error, However when running my host application with 7z.so it happens all the time. We can only guess why an object allocated with "new" contains uninitialized memory in one case but not in the other, most likely this is valgrind who treats this memory as such. The fact is that without either of suggested patches at some point the object really contains some random numbers, resulting in a crash at some point.

    For now I will include both patches into my development and will be waiting for the next release.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks