Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Memory] Valgrind notify "Use of uninitialised value" and "Conditional jump or move depends on uninitialised value" #3089

Closed
nam-leduc opened this issue Sep 5, 2020 · 24 comments
Milestone

Comments

@nam-leduc
Copy link
Contributor

Hi all,

I run valgrind and see the memory issue of tesseract, it is not memory access violation, however, it should be care for making result of running stable.

Environment

Tesseract input/output

Best regards,
Le Duc. Nam

@stweil stweil added the bug label Sep 5, 2020
@stweil
Copy link
Member

stweil commented Sep 5, 2020

All of them are related to Leptonica, so we need to check whether latest Leptonica still shows them or not.

@stweil stweil added this to the 5.0.0 milestone Sep 5, 2020
@zdenop
Copy link
Contributor

zdenop commented Sep 5, 2020

leptonica 1.75 is very old. Use the latest version 1.78.

@nam-leduc
Copy link
Contributor Author

Hi @zdenop and @stweil ,

I would like to attach result of leptonica 1.80 (latest version of leptonica - http://www.leptonica.org/download.html)

Result of tesseract

  • valgrind-memory-log.txt
  • Function in leptonica are called by tesseract meet problems:
    • lept: bmfCreate (called from tesseract::Tesseract::Tesseract)
    • lept: pixGenerateHalftoneMask (called from tesseract::ImageFind::FindImages)
    • lept: pixExpandReplicate (called from tesseract::ImageFind::FindImages)

Result when run with leptonica functions

  • I ran with following function
    • lept: bmfCreate
    • lept: pixGenerateHalftoneMask
    • lept: pixExpandReplicate
  • Following are source code of testing:
    #include "leptonica/allheaders.h"
    
    int main(int argc, char **argv) {
      char* lept_version = getLeptonicaVersion();
      printf("lept_version: %s\n", lept_version);
      free(lept_version);
    
      Pix* img = pixRead("/home/namld/vmshare/have-image.PNG");
      Pix* img_binary = pixConvertTo1(img, 127);
      Pix* img_binary_halftone_mask = pixGenerateHalftoneMask(img_binary, NULL, NULL, NULL);
      Pix* img_binary_expaned = pixExpandReplicate(img_binary, 2);
      L_Bmf* fonts_ = bmfCreate(nullptr, 14);
      bmfDestroy(&fonts_);
      pixDestroy(&img_binary_expaned);
      pixDestroy(&img_binary_halftone_mask);
      pixDestroy(&img_binary);
      pixDestroy(&img);
      return 0;
    }
  • Following are result of leptonica:
    • leptonica-log.txt
    • Function in leptonica meet problems:
      • lept: bmfCreate
      • lept: pixGenerateHalftoneMask
      • lept: pixExpandReplicate --> this function don't have memory issue when run in above source code

Discussion

As my view point, different version of leptonica will have different memory issues, however, if user of tesseract know what version of leptonica using by tesseract without memory issues, that will be very good for user.

Best regards,
Le Duc. Nam

@stweil
Copy link
Member

stweil commented Sep 5, 2020

Thank you for the new test results. CC @DanBloomberg who maintains Leptonica.

Repeating the test with a debug build of Leptonica and the test program would also show the problematic lines of code.

@DanBloomberg
Copy link

Thank you for this detailed report, includling the short program surfacing the errors.
The files were not compiled with debug, so no line numbers are in your valgrind output, which makes it hard to debug from your output.

I was unable to reproduce your error, perhaps because you didn't include the image "have-image.PNG".
Instead, I ran your example using the only image I could find with this issue; namely, this image:
https://raw.githubusercontent.com/tesseract-ocr/tessdata/master/eng.traineddata
with the text "Condition of Guitar" at the top.
Valgrind completed with no errors:

lept_version: leptonica-1.81.0
==1678451==
==1678451== HEAP SUMMARY:
==1678451== in use at exit: 0 bytes in 0 blocks
==1678451== total heap usage: 1,777 allocs, 1,777 frees, 7,617,339 bytes allocated
==1678451==
==1678451== All heap blocks were freed -- no leaks are possible
==1678451==
==1678451== For lists of detected and suppressed errors, rerun with: -s
==1678451== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

(note: version 1.81.0 isn't released; it's what you get from the github master)

Was that the image you used in "have-image.PNG"?
I see that the number of allocations (1777) and bytes allocated (7.617KB) is nearly the same as your log (1779 allocs, 7.614KB allocated).
I verified that I get the identical result with 1.80.0.

@nam-leduc
Copy link
Contributor Author

Hi @DanBloomberg ,

I would like to send leptonica-1.80 result, running information and image.
leptonica180-memory.log
https://user-images.githubusercontent.com/8704662/92315953-ac120e80-f017-11ea-98d1-230c1851007f.PNG

  • Ubuntu:
namld@sunny:~/prjs/tmp$ cat /etc/lsb-release 
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=18.04
DISTRIB_CODENAME=bionic
DISTRIB_DESCRIPTION="Ubuntu 18.04.4 LTS"
  • Valgrind:
namld@sunny:~/prjs/tmp$ valgrind --version
valgrind-3.16.1

Following are all data:
all_data.zip

Discussion

  • I still see valgrind notification for following function:

    • bmfCreate
    • pixGenerateHalftoneMask
  • However, as for pixExpandReplicate, maybe issue come from other part on tesseract when using this function. (called from tesseract::ImageFind::FindImages)

@nam-leduc
Copy link
Contributor Author

nam-leduc commented Sep 6, 2020

Hi @DanBloomberg ,

  • I already built leptonica 1.80 with debug mode and see following log.
    leptonica180-memory-debug.txt

  • I still see valgrind notification for following function:

    • bmfCreate
    • pixGenerateHalftoneMask

Thank you for your efforts!

@stweil
Copy link
Member

stweil commented Sep 6, 2020

@nam-leduc, I tried your example with Leptonica from git master on latest Ubuntu bionic and could not reproduce the bug.

$ valgrind ./test_lept
==137349== Memcheck, a memory error detector
==137349== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==137349== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==137349== Command: ./test_lept
==137349== 
lept_version: leptonica-1.81.0
==137349== 
==137349== HEAP SUMMARY:
==137349==     in use at exit: 0 bytes in 0 blocks
==137349==   total heap usage: 1,777 allocs, 1,777 frees, 7,617,033 bytes allocated
==137349== 
==137349== All heap blocks were freed -- no leaks are possible
==137349== 
==137349== For counts of detected and suppressed errors, rerun with: -v
==137349== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

@stweil
Copy link
Member

stweil commented Sep 6, 2020

I noticed that you have a different valgrind. I used valgrind-3.13.0.

@stweil stweil removed the bug label Sep 6, 2020
@DanBloomberg
Copy link

DanBloomberg commented Sep 6, 2020 via email

@DanBloomberg
Copy link

I made a stab at fixing this.
@nam-leduc
please take the latest at github (or just seedfill.c from the latest) and see if the problem is resolved.

@nam-leduc
Copy link
Contributor Author

@DanBloomberg , thank for your check, it is strange for me, I will test it with your valgrind version and latest version of leptonica.

My valgrind version are 3.16.1 (on above information).

namld@sunny:~/prjs/tmp$ valgrind --version
valgrind-3.16.1

Maybe more than 12h later, because now, I can't access to home's PC.

@DanBloomberg
Copy link

note that it may now be fixed. To test as it was, remove the call to
pixSetPadBits()
around l.269 in seedfill.c

@nam-leduc
Copy link
Contributor Author

Hi @DanBloomberg , it still be strange for me.
I tested with:

and it still meet the problem

leptonica-da4bcd7.log

I will change my virtual machine, re-test it and report later.

@nam-leduc
Copy link
Contributor Author

Hi @DanBloomberg , I used docker container 18.04 and still meet the problem.

docker-command.log
docker-create-container-command.log
leptonica-on-docker-18.04.log

What ubuntu version did you test?

@stweil
Copy link
Member

stweil commented Sep 7, 2020

I see that you used a cmake build while I used autotools. That's a difference, although it should not cause such errors.

My test was with Ubuntu 18.04, no docker, leptonica built using ./configure CFLAGS="-g -O0" --disable-shared && make, executable linked against static library with g++ -g -o lept_test note1/lept_test.cc leptonica/src/.libs/liblept.a -lz -lpng -ljpeg -lgif -ltiff -lwebp.

@DanBloomberg
Copy link

Likewise with Ubuntu 18.04. I used the static makefile. g++ (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0

@nam-leduc Does it still reproduce with the change to seedfill.c?

@nam-leduc
Copy link
Contributor Author

nam-leduc commented Sep 7, 2020

Hi @stweil and @DanBloomberg , following are result when I use command of @stweil
static--g-O0-autotools.log

problems disappeared --> it's strange again. 👶

I tried with CFLAGS="-g" instead of CFLAGS="-g -O0", however, problems still disappeared

Could you please check with cmake build?

Edit:
I used dynamic--g-autotools also and result are problems disappeared
dynamic--g-autotools.log

@DanBloomberg
Copy link

I verify that I get the error messages when building with cmake.
Setting the pad bits makes no difference -- the error messages are still there.

Why would building with cmake be different from autotools?

@DanBloomberg
Copy link

Succeeded in getting the errors with make using the static makefile, on a simpler case:

pix2 = pixRead("uninit.png");
pix3 = pixOpenBrick(NULL, pix2, 5, 5);
pix4 = pixCloseSafeBrick(NULL, pix2, 4, 4);
pix5 = pixSeedfillBinary(NULL, pix3, pix4, 4);

with uninit.png
uninit
attached.

However, this only happens using the default (optimizing) compiler. If you build with debug, there is no error.
So with debug, it is initializing the memory.

I need to find where the memory is not being initialized. It may be in the morphological operations.

@DanBloomberg
Copy link

As I suspected, the problem was in the "safe" closing operation.
This morphological operation adds a sufficiently large border to the input image so that the initial dilation stays within the border. After the erosion, it removes the added border pixels.
I was allocating these new images uninitialized, which made sense because I was going to set the border pixels and blit the source into the frame for the adding step.
By initializing all pixels in the new images, both when adding and removing the border, the problem is fixed.
The problem originates with the pad bits, as one would expect, but in a strange way.
I was not able to fix it by just setting the pad bits, because the rasterop DOESN'T COPY THE PAD BITS. So unless those pixels were initialized when the pix was made, they are never initialized, and they sit in the interior of the image with added border. When the border is removed, those uninitialized pad bits are not copied to the final image. (But I don't know why that's a problem for seedfill after I've set them.)

@DanBloomberg
Copy link

@nam-leduc
It should be OK now. Can you verify and if so, close the bug.
Thank you again for all the effort you put in on this.

@nam-leduc
Copy link
Contributor Author

Hi @DanBloomberg , thank you for your fix. I already seen your fix, however, I don't go home today, so I can't test it again. I will comeback and show you the test information later.

@nam-leduc
Copy link
Contributor Author

Hi @DanBloomberg , thank you so much for your fix. I would like to confirm that this bug issue don't happen on latest version of leptonica DanBloomberg/leptonica@d276e15

Hi @stweil and @DanBloomberg , Additionally I also tested with tesseract and don't see issue of this bug.
Thank for your effort.! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants