1020571 - setParaRunsOnly in icu/source/common/ubidi.c leaks |runsOnlyMemory| sometimes

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Andrew McCreight [:mccr8]]

If you hit the third |goto cleanup3| in this function, then |runsOnlyMemory| has been allocated successfully, but it isn't freed:

    if(U_FAILURE(*pErrorCode)) {
        goto cleanup3;
    }

Presumably |runsOnlyMemory| should be freed before the goto.

Comment 1

[Jinank Jain[:jj]]

Attachment: a1.diff

Does this thing work?

Comment 2

[Jinank Jain[:jj]]

Attachment: a1.diff

Does this thing work?

Comment 3

[Jeff Walden [:Waldo]]

We try not to fix bugs in ICU if we can help it.  They're an "upstream" of us: they do work, and periodically we import it.  They're the subject experts, and we do well to leave work to them (because there's a fair chance we might get it wrong).

The general rule for ICU is that we don't change it unless we've submitted the patch upstream and they've accepted and landed it, and it's absolutely necessary for us to have the patch.  Historically this mostly meant we took build-system things in order to use ICU at all, but they've adopted those.  So now, the set of patches we apply to ICU is super-small, limited to those listed near the end of intl/update-icu.sh, and it's basically high-value things.  One actual feature, and otherwise it's minor (but important) build system changes.

I'm inclined to say the value of fixing one random memory leak doesn't justify the maintenance burden of carrying our own patch.  It'd be better to submit the patch upstream, then we'll pick it up the next time we update ICU.  (Which unfortunately may be some time, due to us wanting to support WinXP and ICU not wanting to any more.  I don't have a good answer to that problem, but I'm hoping we can just wait til Firefox drops WinXP support before it becomes a real problem.)

But for now, that decision can be punted, so I'm going to.  :-)  Please check if upstream's fixed this, and file a bug https://ssl.icu-project.org/trac with patch if it hasn't.  If and when they accept it, we can decide whether or not to apply the patch in our own tree.

Comment 4

[Jinank Jain[:jj]]

I had no idea about that it works like that.
I had filled a bug at icu: https://ssl.icu-project.org/trac/ticket/12649

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Jinank Jain from comment #4)
> I had no idea about that it works like that.

No worries.  Few enough people want to change intl/icu, IMO, that it's not a big deal that this policy isn't well-documented anywhere.  (Not that documentation would be bad -- it's just not super-important.  And to be honest, I don't even know where documentation for this would go, given that you couldn't expect patch authors to read, say, intl/icu/README or similar to learn of such policy.)  Also, practically, patching attempts go through Bugzilla and through this component, in all likelihood, so they'll get scrutiny *eventually* on the point, even if not necessarily pre-patching scrutiny.

Please report back once a patch has been accepted upstream, then we'll talk.  :-)

Comment 6

[Jinank Jain[:jj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> (In reply to Jinank Jain from comment #4)
> > I had no idea about that it works like that.
> 
> No worries.  Few enough people want to change intl/icu, IMO, that it's not a
> big deal that this policy isn't well-documented anywhere.  (Not that
> documentation would be bad -- it's just not super-important.  And to be
> honest, I don't even know where documentation for this would go, given that
> you couldn't expect patch authors to read, say, intl/icu/README or similar
> to learn of such policy.)  Also, practically, patching attempts go through
> Bugzilla and through this component, in all likelihood, so they'll get
> scrutiny *eventually* on the point, even if not necessarily pre-patching
> scrutiny.
> 
> Please report back once a patch has been accepted upstream, then we'll talk.
> :-)

Yeah sure :)

Comment 7

[Nicholas Nethercote [inactive]]

> I don't even know where documentation for this would go, given that
> you couldn't expect patch authors to read, say, intl/icu/README or similar
> to learn of such policy.

I write patches that affect code all over the tree. I regularly look for files like this, and I am grateful when they are present. There are 16 files called README.mozilla in the tree, including one in intl/hyphenation/.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8774027 [details] [diff] [review]
a1.diff

It sounds like Waldo helped you out here. Thanks for your patch, and looking into the upstreaming process. Sorry it is a pain.

Comment 9

[André Bargull [:anba]]

https://unicode-org.atlassian.net/browse/ICU-12649 was fixed in https://unicode-org.atlassian.net/browse/ICU-11177, which shipped in ICU 55 (bug 1075758).