691240 - Apparent leaks in hunspell hash manager

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Justin Lebar (not reading bugmail)]

I'm trying to figure out how this might possibly not be a memory leak.

In hunspell/src/hashmgr.cpp:

int HashMgr::remove(const char * word)
{
    struct hentry * dp = lookup(word);
    while (dp) {
        if (dp->alen == 0 || !TESTAFF(dp->astr, forbiddenword, dp->alen)) {
            unsigned short * flags =
                (unsigned short *) malloc(sizeof(short) * (dp->alen + 1));
            if (!flags) return 1;
            for (int i = 0; i < dp->alen; i++) flags[i] = dp->astr[i];
            flags[dp->alen] = forbiddenword;
            dp->astr = flags;  // XXX shouldn't we be freeing something?
            dp->alen++;
            flag_qsort(flags, 0, dp->alen);
        }
        dp = dp->next_homonym;
    }
    return 0;
}

What this does, apparently, is look up the word we're removing and check whether the word has the "forbiddenword" flag attached to it.  If it does, then we move on.  If it doesn't, then we malloc space for the flags (this itself seems rather wasteful; see bug 691176), add the new flag to the list, and then assign the flags to the word structure.

But it appears that we never free the old value of dp->astr before assigning flags into it.  Assuming this is malloc'ed elsewhere (it sure appears to be), this appears to be a leak.

Elsewhere dp->astr is freed before being assigned into.  See hashmgr.cpp:226, :245.  But a similar, apparently leaking idiom appears on line 346.

Comment 1

[Justin Lebar (not reading bugmail)]

I filed a hunspell bug: https://sourceforge.net/tracker/?func=detail&aid=3417893&group_id=143754&atid=756395

Comment 2

[Justin Lebar (not reading bugmail)]

These may not affect us at Mozilla.

HashMgr::remove eventually becomes the public method Hunspell_remove, which we don't call.  HashMgr::remove_forbidden_flag is called from HashMgr::add, which presumably we do call, but it only leaks when the word has been previously removed.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch I can't properly test.

Comment 4

[Dimitrij Mijoski]

This is fixed in 1.5.x https://github.com/hunspell/hunspell/issues/191

Comment 5

[Dimitrij Mijoski]

We can close this since 1.5 landed in Mozilla tree.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Thanks for checking!