Merge lp:~gz/bzr/create_delta_index_memoryerror_633336 into lp:bzr
Proposed by
Martin Packman
Status: | Merged |
---|---|
Merged at revision: | 5731 |
Proposed branch: | lp:~gz/bzr/create_delta_index_memoryerror_633336 |
Merge into: | lp:bzr |
Diff against target: |
134 lines (+26/-16) 2 files modified
bzrlib/_groupcompress_pyx.pyx (+14/-6) bzrlib/diff-delta.c (+12/-10) |
To merge this branch: | bzr merge lp:~gz/bzr/create_delta_index_memoryerror_633336 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
bzr-core | Pending | ||
Review via email: mp+52251@code.launchpad.net |
Description of the change
Adapt delta_index functions in diff-delta.c to always return a struct pointer on success, even if it's not a newly allocated one. This allows the callers in _groupcompress_pyx to treat NULL returns as a malloc failure and raise MemoryError. Some parameter preconditions are also asserted in the pyrex source just to make sure. This won't help people who are running out of memory during groupcompress, but at least they should know about it.
I'd like to test this, but short of forking and messing around with rlimit I don't have any good ideas.
To post a comment you must log in.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 3/4/2011 9:17 PM, Martin [gz] wrote: _groupcompress_ pyx.DeltaIndex. _populate_ first_index /bugs.launchpad .net/bugs/ 633336 /code.launchpad .net/~gz/ bzr/create_ delta_index_ memoryerror_ 633336/ +merge/ 52251
> Martin [gz] has proposed merging lp:~gz/bzr/create_delta_index_memoryerror_633336 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #633336 AssertionError in bzrlib.
> https:/
>
> For more details, see:
> https:/
>
> Adapt delta_index functions in diff-delta.c to always return a struct pointer on success, even if it's not a newly allocated one. This allows the callers in _groupcompress_pyx to treat NULL returns as a malloc failure and raise MemoryError. Some parameter preconditions are also asserted in the pyrex source just to make sure. This won't help people who are running out of memory during groupcompress, but at least they should know about it.
>
> I'd like to test this, but short of forking and messing around with rlimit I don't have any good ideas.
I'd like to avoid asserts like this:
+ assert src.buf and src.size and self._index
Specifically, if it trips, the user gets back:
AssertionError
With no context, and a traceback, which points to a line. But on that
line, all we know is that *one* of those three cases failed.
If you are comfortable with it, remove it entirely. Alt 1 is to move it
into 3 lines, alt 2 is to move it into:
if not src.buf: "src.buf is None, but should be valid")
raise AssertionError(
etc.
This one the same thing:
+ assert src.size and src.buf
&cp_size)
failed = 1
break
memcpy( out, source + cp_off, cp_size)
if (cp_off + cp_size < cp_size or
- - cp_off + cp_size > source_size or
- - cp_size > size):
+ cp_off + cp_size > <unsigned int>source_size or
+ cp_size > <unsigned int>size):
^- I assume this is more about silencing compiler warnings. I think
there was some question about the 'right' fix, but source_size should
never be negative, and neither should size, so this seems reasonable to me.
- - * Rewritten for GIT by Nicolas Pitre <email address hidden>, (C) 2005-2007
+ * Rewritten for GIT by Nicolas Pitre <email address hidden>, (C) 2005-2007
^- Interesting, haven't seen anything about that before...
- - if (old) {
- - old->last_src = src;
- - }
^- Is last_src still getting set in another code path? I don't remember
exactly where it mattered, but it was important at one time.
+ assert(old_index != NULL);
+ /* GZ 2011-03-04: What is 'something' exactly? If this an unrecoverable
free( entries) ;
+ error perhaps it should be an assert? */
if (data != top) {
/* Something was wrong with this delta */
- - return NULL;
+ return old_index;
}
^- We need to die here. Not carry on as if everything went ok. It means
that the delta was incorrectly formatted. Something like having an
insert of 10 bytes, but only 8 bytes available, etc.
Unfortunately, we only have 1 bit of information to hand...