heap-use-after-free in Datasheet::pinSearch

[krasin]

uconfig crashes with heap-use-after-free error when running on LM393 datasheet from https://lcsc.com/product-detail/Analog-Comparators_ON-Semicon_LM393DR2G_ON-Semicon-ON-LM393DR2G_C7955.html

To reproduce:

1. Checkout bfd75d012583308094deb20190f885d4ffb6dce8 commit (master at the time of writing this).
2. Build uconfig with Address Sanitizer (https://en.wikipedia.org/wiki/AddressSanitizer) enabled:

cd uConfig
mkdir build
cd build
qmake ../src/uConfig.pro CONFIG+=sanitizer CONFIG+=sanitize_address CONFIG+=force_debug_info
make -j4

3. Download the datasheet from lcsc:

curl https://datasheet.lcsc.com/szlcsc/ON-Semicon-ON-LM393DR2G_C7955.pdf > /tmp/C7955.pdf

4. Run uconfig and observe the crash report. Note: tune ASAN_SYMBOLIZER_PATH, if llvm-symbolizer is installed into a different directory on your machine.

$ ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer ./uconfig /tmp/C7955.pdf -o /tmp/C7955.lib
=================================================================
==3852==ERROR: AddressSanitizer: heap-use-after-free on address 0x60b0001ad778 at pc 0x7faf4b6057bd bp 0x7fff1f3dd7d0 sp 0x7fff1f3dd7c0
READ of size 8 at 0x60b0001ad778 thread T0
    #0 0x7faf4b6057bc in Datasheet::pinSearch(int) ../../src/pdf_extract/datasheet.cpp:230
    #1 0x7faf4b6061a8 in Datasheet::analyse(int, int) ../../src/pdf_extract/datasheet.cpp:666
    #2 0x5642b126dc18 in processFilePdf(QString, Lib*, bool) ../../src/uconfig/uconfig.cpp:29
    #3 0x5642b126c987 in main ../../src/uconfig/uconfig.cpp:137
    #4 0x7faf4a62c1e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)
    #5 0x5642b126d72d in _start (/home/krasin/src/github.com/Robotips/uConfig/bin/uconfig+0x772d)

0x60b0001ad778 is located 88 bytes inside of 104-byte region [0x60b0001ad720,0x60b0001ad788)
freed by thread T0 here:
    #0 0x7faf4b83d7ff in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x1107ff)
    #1 0x7faf4b60c648 in DatasheetPackage::~DatasheetPackage() ../../src/pdf_extract/datasheetpackage.cpp:28
    #2 0x7faf4b6000f9 in Datasheet::pinSearch(int) ../../src/pdf_extract/datasheet.cpp:234
    #3 0x7faf4b6061a8 in Datasheet::analyse(int, int) ../../src/pdf_extract/datasheet.cpp:666
    #4 0x5642b126dc18 in processFilePdf(QString, Lib*, bool) ../../src/uconfig/uconfig.cpp:29
    #5 0x5642b126c987 in main ../../src/uconfig/uconfig.cpp:137
    #6 0x7faf4a62c1e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)

previously allocated by thread T0 here:
    #0 0x7faf4b83c867 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10f867)
    #1 0x7faf4b5fac1b in Datasheet::extractPins(int) ../../src/pdf_extract/datasheet.cpp:441
    #2 0x7faf4b5fd4a2 in Datasheet::pinSearch(int) ../../src/pdf_extract/datasheet.cpp:105
    #3 0x7faf4b6061a8 in Datasheet::analyse(int, int) ../../src/pdf_extract/datasheet.cpp:666
    #4 0x5642b126dc18 in processFilePdf(QString, Lib*, bool) ../../src/uconfig/uconfig.cpp:29
    #5 0x5642b126c987 in main ../../src/uconfig/uconfig.cpp:137
    #6 0x7faf4a62c1e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)

SUMMARY: AddressSanitizer: heap-use-after-free ../../src/pdf_extract/datasheet.cpp:230 in Datasheet::pinSearch(int)

Note: the same error occurs if built without Address Sanitizer, but it obviously misses the nice report.

The code around the issue is:

foreach (DatasheetPackage *package, packages)
    {
        // package with less than 5 pin are deleted                                                                                 
        if (package->pins.count() < 5)
        {
            badcount++;
            delete package;
            continue;
        }
        // package pin 1 very far to pin 2 are deleted                                                                              
        QPointF p1center = package->pins[1]->numberBox->pos.center(); <==== USE-AFTER-FREE HERE
    if (package->pins[0]->numberBox->distanceToPoint(p1center) > 50)
        {
            badcount++;
            delete package; <==== DELETED HERE
            continue;
        }

What happens is that package (including its pins) is deleted on one iteration and then on the next iteration, a deleted pin is used again. I have not yet fully understood the problem, but I hope this report gives enough clues.

[krasin]

@sebcaux thank you for the awesome tool, btw!

[krasin]

I have played with it a little longer, and it's clear that the same pin gets assigned to two different packages. The one of the packages gets deleted and (surprisingly) the Package destructor also deletes pins (including the shared one), and then this pins gets accessed from another Package.

DatasheetPackage::~DatasheetPackage()
{
  for (int i=0; i<pins.size(); i++) {
    delete pins[i];
  }
}

One way to resolve that is to consider that a Package does not own its pins (so, it does not delete them). Instead, push its ownership to Datasheet. This way, sharing pins across different packages won't be problem.

Another option is to copy pins. Basically, instead of QList<DatasheetPin *> pins; make it QList<DatasheetPin> pins;.

It's also possible that there's a better fix, but I am not familiar enough with the code base to have a good intuition of what fix would be consistent with the rest of the uConfig logic.

Any suggestions are welcome. I would be more than happy to come up with a (tested) pull request, but would like to have a suggestion about what's the preferred course of actions here.