patiencediff tries to malloc 0 bytes; tests fail on AIX

Bug #331095 reported by Cris Boylan on 2009-02-18
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Bazaar
Medium
Martin Pool
2.0
Medium
Unassigned
2.1
Medium
Unassigned

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.

Related branches

Cris Boylan (crispin-boylan) wrote :

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)
Martin Pool (mbp) wrote :

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

Martin Pool (mbp) wrote :

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

Martin Pool (mbp) wrote :

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

> 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.

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/>

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers