Bazaar

patiencediff tries to malloc 0 bytes; tests fail on AIX

Bug #331095 reported by Cris Boylan on 2009-02-18

This bug affects 1 person

	Status	Importance	Assigned to	Milestone
Bazaar	Fix Released	Medium	Martin Pool	Bazaar 2.2b1
2.0	Fix Released	Medium	Unassigned	Bazaar 2.0.5
2.1	Fix Released	Medium	Unassigned	Bazaar 2.1.1

Bug Description

When running patiencediff tests on AIX:

bzrlib.tests.test_diff.TestPatienceDiffLib.test_diff_unicode_string
bzrlib.tests.test_diff.TestPatienceDiffLib.test_grouped_opcodes
bzrlib.tests.test_diff.TestPatienceDiffLib.test_matching_blocks
bzrlib.tests.test_diff.TestPatienceDiffLib.test_matching_blocks_tuples
bzrlib.tests.test_diff.TestPatienceDiffLib.test_multiple_ranges
bzrlib.tests.test_diff.TestPatienceDiffLib.test_opcodes
bzrlib.tests.test_diff.TestPatienceDiffLib.test_patience_unified_diff
bzrlib.tests.test_diff.TestPatienceDiffLib.test_patience_unified_diff_with_dates
bzrlib.tests.test_diff.TestPatienceDiffLib.test_recurse_matches
bzrlib.tests.test_diff.TestPatienceDiffLib.test_unique_lcs
bzrlib.tests.test_diff.TestPatienceDiffLibFiles.test_patience_unified_diff_files
bzrlib.tests.test_diff.TestPatienceDiffLibFiles_c.test_patience_unified_diff_files
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_diff_unicode_string
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_grouped_opcodes
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_matching_blocks
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_matching_blocks_tuples
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_multiple_ranges
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_opcodes
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_patience_unified_diff
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_patience_unified_diff_with_dates
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_recurse_matches
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_unhashable
bzrlib.tests.test_diff.TestPatienceDiffLib_c.test_unique_lcs
bzrlib.tests.test_diff.TestUsingCompiledIfAvailable.test_PatienceSequenceMatcher

all these fail with MemoryError.

It turns out that the patiencediff extension occasionally tries to malloc 0 bytes. On AIX this returns NULL which is not guarded against (but is in the spec). Patiencediff code counts this as a failure to allocate memory.

I have attached a patch which stops this happening (basically just checks if we were malloc'ing zero bytes as well). With this change all tests pass successfully.

Tags:

Related branches

lp:~mbp/bzr/331095-malloc

Merged into lp:bzr

Andrew Bennetts: Approve on 2010-02-11

lp:~mbp/bzr/331095-malloc-2.0

Merged into lp:bzr/2.0

John A Meinel: Approve on 2010-02-23

Revision history for this message

Cris Boylan (crispin-boylan) wrote on 2009-02-18:

bzr-1.12-patiencediff.patch Edit (2.0 KiB, text/plain)

Revision history for this message

John A Meinel (jameinel) wrote on 2009-02-19: Re: patiencediff tests fail on AIX

This is ok, but I think a better patch would just recognize when we are trying to do a zero-length comparison, and stop trying at that point. Are you able to put something like that together?

Martin Pool (mbp) on 2009-06-22

summary:	- patiencediff tests fail on AIX + patiencediff tries to malloc 0 bytes; tests fail on AIX
Changed in bzr:
importance:	Undecided → Low
status:	New → In Progress

Martin Pool (mbp) on 2010-02-11

tags:

added: aix pyrex sunos

Martin Pool (mbp) on 2010-02-11

Changed in bzr:
importance:	Low → Medium
assignee:	nobody → Martin Pool (mbp)

Revision history for this message

Martin Pool (mbp) wrote on 2010-02-11:

trap-malloc.diff Edit (329 bytes, text/plain)

This patch makes the failures reproducible on Linux.

At least one failure is in line PatienceSequenceMatcher_new, which wants to allocate 'backpointers' proportional to the number of lines in b, which fails if b is empty. So the choices there seem to be:

* leave backpointers as null if b is empty; any attempt to access it will crash fairly obviously but we must cope if it's null when freeing it
* allocate at least one byte (a bit gross) so that every platform works like gnu libc

Revision history for this message

Martin Pool (mbp) wrote on 2010-02-11:

Actually it seems it is portable that free(NULL) is a no-op, so #1 is the way to go.

Revision history for this message

Martin Pool (mbp) wrote on 2010-02-11:

After checking for fallout in trunk we can merge https://code.edge.launchpad.net/~mbp/bzr/331095-malloc-2.0

Changed in bzr:
milestone:	none → 2.2.0b1
status:	In Progress → Fix Released

Revision history for this message

Matthew Fuller (fullermd) wrote on 2010-02-11: Re: [Bug 331095] Re: patiencediff tries to malloc 0 bytes; tests fail on AIX

> Actually it seems it is portable that free(NULL) is a no-op, so #1
> is the way to go.

It's common, but not necessary; free(NULL) falls into undefined
behavior, and it's possible that some cases may cause nasal demons.
Older versions of dmalloc errored by default on it, for instance.

That said, I don't offhand know of any current or not-so-current
system malloc implementation that chokes on it.

Revision history for this message

Martin Pool (mbp) wrote on 2010-02-11: Re: [Bug 331095] Re: patiencediff tries to malloc 0 bytes; tests fail on AIX

On 11 February 2010 20:39, fullermd <email address hidden> wrote:
>> Actually it seems it is portable that free(NULL) is a no-op, so #1
>> is the way to go.
>
> It's common, but not necessary; free(NULL) falls into undefined
> behavior, and it's possible that some cases may cause nasal demons.
> Older versions of dmalloc errored by default on it, for instance.
>
> That said, I don't offhand know of any current or not-so-current
> system malloc implementation that chokes on it.

If it happens it will be obvious.

It looks like we're already counting on that behaviour anyhow.

--
Martin <http://launchpad.net/~mbp/>