-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/4/2011 9:17 PM, Martin [gz] wrote:
> 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._groupcompress_pyx.DeltaIndex._populate_first_index
>   https://bugs.launchpad.net/bugs/633336
> 
> For more details, see:
> https://code.launchpad.net/~gz/bzr/create_delta_index_memoryerror_633336/+merge/52251
> 
> 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:
  raise AssertionError("src.buf is None, but should be valid")

etc.


This one the same thing:
+            assert src.size and src.buf


&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):
                     failed = 1
                     break
                 memcpy(out, source + cp_off, cp_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
+                      error perhaps it should be an assert? */
     if (data != top) {
         /* Something was wrong with this delta */
         free(entries);
- -        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 back to the
caller at this point. NULL or new index. But really, this change is not
sufficient. And assert in C code will probably cause SIGABRT, or worse,
is a no-op because of -O3 compile-time flags. Neither is great for this
condition. Thoughts?


The other cases you've found are fine to be changed in the way you did.
I'd rather get a MemoryError than accept bogus data, and have that get
committed to the repository. a forceful assert that *always* triggered
self-destruction would almost be as good, since we would again avoid
inserting corrupted data into the repository. Though we'd leave cruft
around on disk, etc. (Raising an exception should cause 'finally'
clauses to unlock, deleted the affected pack file, etc.)

Thoughts on how to send back a more tasteful error? Otherwise I like the
idea, but this one in particular is a bit of an ugly side-case.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk1xXesACgkQJdeBCYSNAAN2HACgibESJWu+gGkEMFjU0tFv/BLn
L1UAnjk3AdvoZyuHrrz7YO5DAALMoVhD
=1mHQ
-----END PGP SIGNATURE-----