search

mmorise / World

A high-quality speech analysis, manipulation and synthesis system
http://www.kisc.meiji.ac.jp/~mmorise/world/english
Other
1.18k stars 253 forks source link

Harvest: two memory fixes that may have caused undefined behavior or segfaults #100

Closed r9y9 closed 4 years ago

r9y9 commented 4 years ago

This will fix potential undefined behavior (negative F0 or higher F0 over f0_ceil, those are what I see in fact!) and segfault issues when we use Harvest. Please see the individual commit message for details.

For reference, here are the analysis results with clang memory sanitizer (https://clang.llvm.org/docs/MemorySanitizer.html):

Without https://github.com/mmorise/World/commit/66e5dcdfcd368f21429d815ad6bf9b14aa9d46a8:

$ ./test ~/sp/nnsvs/egs/nit-song070/00-svs-world/data/acoustic/wav/nitech_jp_song070_f001_012.wav
File information
Sampling : 48000 Hz 16 Bit
Length 4262397 [sample]
Length 88.799937 [sec]

==7083==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x53e1a8 in (anonymous namespace)::GetBoundaryList(double const*, int, int*) /home/ryuichi/sp/world-cmake/src/harvest.cpp:739:9
    #1 0x53b83f in (anonymous namespace)::FixStep2(double const*, int, int, double*) /home/ryuichi/sp/world-cmake/src/harvest.cpp:756:5
    #2 0x525d4b in (anonymous namespace)::FixF0Contour(double const* const*, double const* const*, int, int, double*) /home/ryuichi/sp/world-cmake/src/harvest.cpp:1040:3
    #3 0x521f1a in (anonymous namespace)::HarvestGeneralBody(double const*, int, int, int, double, double, double, int, double*, double*) /home/ryuichi/sp/world-cmake/src/harvest.cpp:1204:3
    #4 0x51fc5d in Harvest /home/ryuichi/sp/world-cmake/src/harvest.cpp:1245:3
    #5 0x4aa9a5 in (anonymous namespace)::F0EstimationHarvest(double*, int, WorldParameters*) /home/ryuichi/sp/world-cmake/test/test.cpp:157:3
    #6 0x4a9108 in main /home/ryuichi/sp/world-cmake/test/test.cpp:398:3
    #7 0x7fd2acd8f82f in __libc_start_main /build/glibc-LK5gWL/glibc-2.23/csu/../csu/libc-start.c:291
    #8 0x41c1a8 in _start (/home/ryuichi/Dropbox/sp/world-cmake/build/test+0x41c1a8)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ryuichi/sp/world-cmake/src/harvest.cpp:739:9 in (anonymous namespace)::GetBoundaryList(double const*, int, int*)

Without https://github.com/mmorise/World/commit/33159af1e3e7d7c5b626fb34a7d675c74193a624:

$ ./test ~/sp/nnsvs/egs/nit-song070/00-svs-world/data/acoustic/wav/nitech_jp_song070_f001_012.wav
File information
Sampling : 48000 Hz 16 Bit
Length 4262397 [sample]
Length 88.799937 [sec]

Analysis
==8281==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x53a180 in (anonymous namespace)::SelectBestF0(double, double const*, int, double, double*) /home/ryuichi/sp/world-cmake/src/harvest.cpp:644:9
    #1 0x5398f9 in (anonymous namespace)::RemoveUnreliableCandidatesSub(int, int, double const* const*, int, double**, double**) /home/ryuichi/sp/world-cmake/src/harvest.cpp:659:3
    #2 0x52552a in (anonymous namespace)::RemoveUnreliableCandidates(int, int, double**, double**) /home/ryuichi/sp/world-cmake/src/harvest.cpp:683:7
    #3 0x521d84 in (anonymous namespace)::HarvestGeneralBody(double const*, int, int, int, double, double, double, int, double*, double*) /home/ryuichi/sp/world-cmake/src/harvest.cpp:1200:3
    #4 0x51fc5d in Harvest /home/ryuichi/sp/world-cmake/src/harvest.cpp:1245:3
    #5 0x4aa9a5 in (anonymous namespace)::F0EstimationHarvest(double*, int, WorldParameters*) /home/ryuichi/sp/world-cmake/test/test.cpp:157:3
    #6 0x4a9108 in main /home/ryuichi/sp/world-cmake/test/test.cpp:398:3
    #7 0x7fe5d070782f in __libc_start_main /build/glibc-LK5gWL/glibc-2.23/csu/../csu/libc-start.c:291
    #8 0x41c1a8 in _start (/home/ryuichi/Dropbox/sp/world-cmake/build/test+0x41c1a8)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ryuichi/sp/world-cmake/src/harvest.cpp:644:9 in (anonymous namespace)::SelectBestF0(double, double const*, int, double, double*)

Ref https://github.com/r9y9/nnsvs/issues/7.

With these two fixes, no issues are found with clang address/memory sanitizers, so there should be no memory/address issues.

mmorise commented 4 years ago

Thank you for your request.

I think that the revision in line 677 is reasonable, so I will accept it. In the revision in line 715, I propose another solution. The following provides the same result compared with your request. I'd like to revise the line 712 as the following. for (int i = 0; i < f0_length; ++i) f0_step1[i] = 0.0; I think that this is shorter than your idea.

If possible, please revise your request in line 715. I will accept the request.

r9y9 commented 4 years ago

Thank you for the review! I have pushed the changes based on your comments.

mmorise commented 4 years ago

Thank you for your revision!