#206 FastICA accesses unallocated memory/ bails out if whitenig

release_4.2.x
closed
5
2013-05-24
2012-05-16
Gert Wollny
No

In the first step, FastICA runs a whitening that may return a matrix with a lower dimension then the requested number of independent components.
If the library is compiled with debug, this will trigger an assertion failure, and without the debug flag, unallocated memory is accessed.

Reproducible: Always

Steps to Reproduce:
A test case is attached that triggers the bug. To see the bug either compile the library with debug use flag enabled or run it with valgrind.
Actual Results:
Assertion fails or unallocated memory is accessed.

Expected Results:
test passes

See also In the first step, FastICA runs a whitening that may return a matrix with a lower dimension then the requested number of independent components.
If the library is compiled with debug, this will trigger an assertion failure, and without the debug flag, unallocated memory is accessed.

Reproducible: Always

Steps to Reproduce:
A test case is attached that triggers the bug. To see the bug either compile the library with debug use flag enabled or run it with valgrind.
Actual Results:
Assertion fails or unallocated memory is accessed.

Expected Results:
test passes

See also: https://bugs.gentoo.org/show_bug.cgi?id=370475 and http://sourceforge.net/projects/itpp/forums/forum/115656/topic/4389287

Discussion

  • Gert Wollny

    Gert Wollny - 2012-05-16

    Test case that triggers the bug.

     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2012-05-25

    Thank you for this patch!
    A border case of the same issue is when the input data is all-zero and no principle component can be found. i.e., if you feed all-zero channels into Fast_ICA, you reap another assert/segfault.

    I have extended above patch to return the input data as-is if pcamat() ends up with zero dimensions. pcamat() is changed to return the number of components, and Fast_ICA::separate() then bails out early if that is zero. (going to attach a patch file)

     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2012-05-25

    (couldn't attach a file, here's the patch as comment)

    Index: itpp/signal/fastica.cpp

    --- itpp/signal/fastica.cpp (revision 1886)
    +++ itpp/signal/fastica.cpp (working copy)
    @@ -82,7 +82,7 @@
    @{
    */
    static void selcol(const mat oldMatrix, const vec maskVector, mat & newMatrix);
    -static void pcamat(const mat vectors, const int numOfIC, int firstEig, int lastEig, mat & Es, vec & Ds);
    +static int pcamat(const mat vectors, const int numOfIC, int firstEig, int lastEig, mat & Es, vec & Ds);
    static void remmean(mat inVectors, mat & outVectors, vec & meanValue);
    static void whitenv(const mat vectors, const mat E, const mat D, mat & newVectors, mat & whiteningMatrix, mat & dewhiteningMatrix);
    static mat orth(const mat A);
    @@ -143,9 +143,17 @@

    remmean(mixedSig, mixedSigC, mixedMean);

    • pcamat(mixedSigC, numOfIC, firstEig, lastEig, E, D);
    • if (pcamat(mixedSigC, numOfIC, firstEig, lastEig, E, D) < 1) {
    • // no principal components could be found (e.g. all-zero data).
    • // Let's return the unchanged input.
    • icasig = mixedSig;
    • return false;
    • }

    whitenv(mixedSigC, E, diag(D), whitesig, whiteningMatrix, dewhiteningMatrix);
    +
    + Dim = whitesig.rows();
    + if (numOfIC > Dim) numOfIC = Dim;

    ivec NcFirst = to_ivec(zeros(numOfIC));
    @@ -161,10 +169,6 @@
    bool result = true;
    if (PCAonly == false) {

    - Dim = whitesig.rows();

    - if (numOfIC > Dim) numOfIC = Dim;

     result = fpica(whitesig, whiteningMatrix, dewhiteningMatrix, approach, numOfIC, g, finetune, a1, a2, mu, stabilization, epsilon, maxNumIterations, maxFineTune, initState, guess, sampleSize, A, W);
    
     icasig = W * mixedSig;
    

    @@ -257,7 +261,7 @@

    }

    -static void pcamat(const mat vectors, const int numOfIC, int firstEig, int lastEig, mat & Es, vec & Ds)
    +static int pcamat(const mat vectors, const int numOfIC, int firstEig, int lastEig, mat & Es, vec & Ds)
    {

    mat Et;
    @@ -277,6 +281,8 @@

    // Compute rank
    for (int i = 0; i < Dt.length(); i++) if (Dt(i) > FICA_TOL) maxLastEig++;
    + if (maxLastEig < 1)
    + return 0;

    // Force numOfIC components
    if (maxLastEig > numOfIC) maxLastEig = numOfIC;
    @@ -324,7 +330,7 @@
    numTaken++;
    }

    • return;
    • return lastEig;

    }

     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2012-05-25

    just commenting on the order sf.net displays stuff -- read this bottum-up:
    Way below is the original patch by the issue submitter, attached as a file. Then above that is my first comment, and above that is my humble extension of the original patch, in a second comment. Just you don't get mixed up...

     
  • Bogdan Cristea

    Bogdan Cristea - 2013-05-23
    • assigned_to: Bogdan Cristea
     
  • Bogdan Cristea

    Bogdan Cristea - 2013-05-23

    changes commited into master branch

     
  • Bogdan Cristea

    Bogdan Cristea - 2013-05-23
    • status: open --> accepted
     
  • Bogdan Cristea

    Bogdan Cristea - 2013-05-24
    • status: accepted --> closed
     

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

Sign up for the SourceForge newsletter:





No, thanks