Merge lp:~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment into lp:akiban-persistit

Proposed by Peter Beaman
Status: Merged
Approved by: Nathan Williams
Approved revision: 427
Merged at revision: 425
Proposed branch: lp:~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment
Merge into: lp:akiban-persistit
Diff against target: 976 lines (+382/-265)
7 files modified
src/main/java/com/persistit/Key.java (+286/-194)
src/main/java/com/persistit/Persistit.java (+1/-1)
src/main/java/com/persistit/Transaction.java (+1/-1)
src/main/java/com/persistit/exception/KeyTooLongException.java (+39/-0)
src/test/java/com/persistit/TransactionAbandonedTest.java (+16/-14)
src/test/java/com/persistit/unit/ExchangeTest.java (+3/-3)
src/test/java/com/persistit/unit/KeyTest1.java (+36/-52)
To merge this branch: bzr merge lp:~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment
Reviewer Review Type Date Requested Status
Nathan Williams Approve
Akiban Build User Needs Fixing
Review via email: mp+154422@code.launchpad.net

Description of the change

Add a test to exercise various places where code in the Key#append methods attempts to write past the end of the encoded byte array.

Add try/catch blocks to all relevant Key#append methods to catch AIOOBE and instead throw newly added KeyTooLongException. KeyTooLongException is unchecked so as not to change lots of use cases where Keys modifications occur.

Strategy is to catch the AIOOBE, which is a rare, unexpected event rather than to add bound-checking code everywhere. Since the array operations already do bounds checking this seems like a better approach.

To post a comment you must log in.
Revision history for this message
Nathan Williams (nwilliams) wrote :

We should always restore _size in the even of an error, right? There are a handful of places that don't do that, such as append(boolean). Perhaps adding the size to restore to the new tooLong() helper method would enforce that nicely.

Otherwise seems simple enough.

review: Needs Information
Revision history for this message
Peter Beaman (pbeaman) wrote :

I took some care to verify that the places that don't restore the value of
_size also don't change it.

On Wed, Mar 20, 2013 at 2:03 PM, Nathan Williams <email address hidden>wrote:

> Review: Needs Information
>
> We should always restore _size in the even of an error, right? There are a
> handful of places that don't do that, such as append(boolean). Perhaps
> adding the size to restore to the new tooLong() helper method would enforce
> that nicely.
>
> Otherwise seems simple enough.
> --
>
> https://code.launchpad.net/~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment/+merge/154422
> You are the owner of
> lp:~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment.
>

Revision history for this message
Peter Beaman (pbeaman) wrote :

Persuaded. Modified all Key#append methods to save starting size and restore it in the tooLong method. As you mentioned, this pattern will help guard against newly added errors.

Revision history for this message
Nathan Williams (nwilliams) wrote :

Thanks Peter!

review: Approve
Revision history for this message
Peter Beaman (pbeaman) wrote :

Found some new problems and pushed back to Needs Review. Basically, the new KeyTest1#testKeyTooLong does not loop correctly and does not actually test all object values in the TEST_OBJECTS array. In particular, the method Key#appendString did not handle AIOOBE, and that's the one we are more concerned about.

Revision history for this message
Akiban Build User (build-akiban) wrote :

There were 2 failures during build/test:

* job server-build failed at build number 3912: http://172.16.20.104:8080/job/server-build/3912/

* view must-pass failed: server-build is aborted

review: Needs Fixing
427. By Peter Beaman

Fix testKeyTooLong. Modify appendString to throw KeyTooLongException rather than ConversionException.

Revision history for this message
Peter Beaman (pbeaman) wrote :

New modifications:

correct KeyTest1#testKeyTooLongException

add try/catch to appendString, appendNull, appendBefore and appendAfter

Revision history for this message
Nathan Williams (nwilliams) wrote :

Looks good, again.

review: Approve
Revision history for this message
Peter Beaman (pbeaman) wrote :

Note on a concern I researched a bit. We mangle the keys in the sort tree. If the server were to use those keys to deliver results from a sort or SELECT DISTINCT (like a covering index) then of course we would return the wrong result. However, I believe this does not happen.

It appears the Sort_Tree operator stores and reads data back from value instances stored in the tree, rather than trying to use the key as a source. Specifically, in PValueSorterAdapter$InternalPAdapter, there is a method putToHolders that copies data from a valueSource to the result row. The valueSource is a PersistitValuePValueSource, not a PersistitKeyPValueSource.

The code path is complex, so I would appreciate confirmation that this is the case. But from my reading I can only see a path that reads from a PersistitValuePValueSource.

Revision history for this message
Mike McMahon (mmcm) wrote :

> The code path is complex, so I would appreciate confirmation that this is the case. But from my reading I can only see a path that reads from a PersistitValuePValueSource.

Yes, we are good.

Look at Sorter.loadTree() and its calls to createKey() and createValue(). The entire incoming Row is encoded in the value and that is always the output source. This allows it to support cases where you sort on a subset of the columns that pass through. No particular attempt is made to suppress duplication (as you say, like a covering index).

Revision history for this message
Peter Beaman (pbeaman) wrote :

The change I made this morning in loadTree (to close the cursor when
there's an Exception) is wrong. Will correct it shortly. I am working on
additional tests.

On Thu, Mar 21, 2013 at 1:24 PM, Mike McMahon <email address hidden> wrote:

> > The code path is complex, so I would appreciate confirmation that this
> is the case. But from my reading I can only see a path that reads from a
> PersistitValuePValueSource.
>
> Yes, we are good.
>
> Look at Sorter.loadTree() and its calls to createKey() and createValue().
> The entire incoming Row is encoded in the value and that is always the
> output source. This allows it to support cases where you sort on a subset
> of the columns that pass through. No particular attempt is made to suppress
> duplication (as you say, like a covering index).
> --
>
> https://code.launchpad.net/~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment/+merge/154422
> You are the owner of
> lp:~pbeaman/akiban-persistit/fix-1157809-aioobe-on-append-key-segment.
>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/main/java/com/persistit/Key.java'
2--- src/main/java/com/persistit/Key.java 2013-01-19 17:42:22 +0000
3+++ src/main/java/com/persistit/Key.java 2013-03-20 19:33:22 +0000
4@@ -27,6 +27,7 @@
5 import com.persistit.encoding.KeyRenderer;
6 import com.persistit.exception.ConversionException;
7 import com.persistit.exception.InvalidKeyException;
8+import com.persistit.exception.KeyTooLongException;
9 import com.persistit.exception.MissingKeySegmentException;
10 import com.persistit.util.Util;
11
12@@ -924,8 +925,6 @@
13 * Key copiedKey = new Key(originalKey);
14 * </pre></code>
15 *
16- *
17- *
18 * @param key
19 * The <code>Key</code> to copy.
20 */
21@@ -947,17 +946,25 @@
22 key.bumpGeneration();
23 }
24
25+ /**
26+ * Construct a <code>Key</code> with a maximum length of
27+ * {@value #MAX_KEY_LENGTH}.
28+ *
29+ * @param persistit
30+ */
31 public Key(final Persistit persistit) {
32 this(persistit, MAX_KEY_LENGTH);
33 }
34
35 /**
36- * Constructs a <code>Key</code> with the specified maximum length.
37+ * Construct a <code>Key</code> with the specified maximum length.
38 *
39+ * @param Persistit
40+ * the Persistit instance
41 * @param maxLength
42 * The maximum length
43 */
44- private Key(final Persistit persistit, final int maxLength) {
45+ public Key(final Persistit persistit, final int maxLength) {
46 _persistit = persistit;
47 if (maxLength <= 0) {
48 throw new IllegalArgumentException("Key length must be positive");
49@@ -1479,13 +1486,11 @@
50 } else if (z0 != 0 && z1 == (byte) 1) {
51 nudged = Nudged.RIGHT;
52 _bytes[_size - 1] = 0;
53- } else if (z0 != 0 && z1 != 0) {
54+ } else if (z0 != 0 && z1 != 0 && _size < _maxSize) {
55 nudged = Nudged.LEFT;
56 save = _bytes[_size];
57 _bytes[_size] = (byte) 0;
58 _size++;
59- // _bytes[_size - 2]++;
60- // _bytes[_size - 1] = (byte) 0;
61 }
62 }
63
64@@ -1539,11 +1544,15 @@
65 * @return This <code>Key</code>, to permit method call chaining
66 */
67 public Key append(final boolean v) {
68- testValidForAppend();
69- int size = _size;
70- _bytes[size++] = v ? (byte) TYPE_BOOLEAN_TRUE : (byte) TYPE_BOOLEAN_FALSE;
71-
72- return endSegment(size);
73+ final int save = _size;
74+ try {
75+ testValidForAppend();
76+ int size = _size;
77+ _bytes[size++] = v ? (byte) TYPE_BOOLEAN_TRUE : (byte) TYPE_BOOLEAN_FALSE;
78+ return endSegment(size);
79+ } catch (final ArrayIndexOutOfBoundsException e) {
80+ return tooLong(save);
81+ }
82 }
83
84 /*
85@@ -1629,21 +1638,26 @@
86 * @return This <code>Key</code>, to permit method call chaining
87 */
88 public Key append(final byte v) {
89- testValidForAppend();
90- int size = _size;
91- if (v > 0) {
92- _bytes[size++] = TYPE_BYTE + EWIDTH_BYTE + 1;
93- _bytes[size++] = (byte) (0x80 | v);
94- } else if (v < 0) {
95- _bytes[size++] = TYPE_BYTE;
96- _bytes[size++] = (byte) (0x80 | v);
97- } else // v == 0
98- {
99- _bytes[size++] = TYPE_BYTE + EWIDTH_BYTE;
100+ final int save = _size;
101+ try {
102+ testValidForAppend();
103+ int size = _size;
104+ if (v > 0) {
105+ _bytes[size++] = TYPE_BYTE + EWIDTH_BYTE + 1;
106+ _bytes[size++] = (byte) (0x80 | v);
107+ } else if (v < 0) {
108+ _bytes[size++] = TYPE_BYTE;
109+ _bytes[size++] = (byte) (0x80 | v);
110+ } else // v == 0
111+ {
112+ _bytes[size++] = TYPE_BYTE + EWIDTH_BYTE;
113+ }
114+ // Close out the segment.
115+
116+ return endSegment(size);
117+ } catch (final ArrayIndexOutOfBoundsException e) {
118+ return tooLong(save);
119 }
120- // Close out the segment.
121-
122- return endSegment(size);
123 }
124
125 /**
126@@ -1654,9 +1668,65 @@
127 * @return This <code>Key</code>, to permit method call chaining
128 */
129 public Key append(final short v) {
130- testValidForAppend();
131- int size = _size;
132- if (v >= 0) {
133+ final int save = _size;
134+ try {
135+ testValidForAppend();
136+ int size = _size;
137+ if (v >= 0) {
138+ int scale = 3;
139+ if (v > 0x3FFF)
140+ scale = 0;
141+ else if (v > 0x007F)
142+ scale = 1;
143+ else if (v > 0x0000)
144+ scale = 2;
145+ _bytes[size++] = (byte) (TYPE_SHORT + EWIDTH_SHORT * 2 - scale);
146+ switch (scale) {
147+ // control falls through intentionally
148+ case 0:
149+ _bytes[size++] = (byte) (0x80 | (v >>> 14));
150+ case 1:
151+ _bytes[size++] = (byte) (0x80 | (v >>> 7));
152+ case 2:
153+ _bytes[size++] = (byte) (0x80 | v);
154+ }
155+ } else {
156+ int scale = 2;
157+ if (v < -0x3FFF)
158+ scale = 0;
159+ else if (v < -0x007F)
160+ scale = 1;
161+ _bytes[size++] = (byte) (TYPE_SHORT + scale);
162+ switch (scale) {
163+ // control falls through intentionally
164+ case 0:
165+ _bytes[size++] = (byte) (0x80 | (v >>> 14));
166+ case 1:
167+ _bytes[size++] = (byte) (0x80 | (v >>> 7));
168+ case 2:
169+ _bytes[size++] = (byte) (0x80 | v);
170+ }
171+ }
172+ // Close out the segment.
173+
174+ return endSegment(size);
175+ } catch (final ArrayIndexOutOfBoundsException e) {
176+ return tooLong(save);
177+ }
178+ }
179+
180+ /**
181+ * Encodes and appends a char value to the key.
182+ *
183+ * @param v
184+ * The char value to append
185+ * @return This <code>Key</code>, to permit method call chaining
186+ */
187+ public Key append(final char v) {
188+ final int save = _size;
189+ try {
190+ testValidForAppend();
191+ int size = _size;
192 int scale = 3;
193 if (v > 0x3FFF)
194 scale = 0;
195@@ -1664,67 +1734,21 @@
196 scale = 1;
197 else if (v > 0x0000)
198 scale = 2;
199- _bytes[size++] = (byte) (TYPE_SHORT + EWIDTH_SHORT * 2 - scale);
200- switch (scale) {
201- // control falls through intentionally
202- case 0:
203- _bytes[size++] = (byte) (0x80 | (v >>> 14));
204- case 1:
205- _bytes[size++] = (byte) (0x80 | (v >>> 7));
206- case 2:
207- _bytes[size++] = (byte) (0x80 | v);
208- }
209- } else {
210- int scale = 2;
211- if (v < -0x3FFF)
212- scale = 0;
213- else if (v < -0x007F)
214- scale = 1;
215- _bytes[size++] = (byte) (TYPE_SHORT + scale);
216- switch (scale) {
217- // control falls through intentionally
218- case 0:
219- _bytes[size++] = (byte) (0x80 | (v >>> 14));
220- case 1:
221- _bytes[size++] = (byte) (0x80 | (v >>> 7));
222- case 2:
223- _bytes[size++] = (byte) (0x80 | v);
224- }
225- }
226- // Close out the segment.
227-
228- return endSegment(size);
229- }
230-
231- /**
232- * Encodes and appends a char value to the key.
233- *
234- * @param v
235- * The char value to append
236- * @return This <code>Key</code>, to permit method call chaining
237- */
238- public Key append(final char v) {
239- testValidForAppend();
240- int size = _size;
241- int scale = 3;
242- if (v > 0x3FFF)
243- scale = 0;
244- else if (v > 0x007F)
245- scale = 1;
246- else if (v > 0x0000)
247- scale = 2;
248- _bytes[size++] = (byte) (TYPE_CHAR + EWIDTH_CHAR - scale);
249- switch (scale) {
250- // control falls through intentionally
251- case 0:
252- _bytes[size++] = (byte) (0x80 | (v >>> 14));
253- case 1:
254- _bytes[size++] = (byte) (0x80 | (v >>> 7));
255- case 2:
256- _bytes[size++] = (byte) (0x80 | v);
257- }
258-
259- return endSegment(size);
260+ _bytes[size++] = (byte) (TYPE_CHAR + EWIDTH_CHAR - scale);
261+ switch (scale) {
262+ // control falls through intentionally
263+ case 0:
264+ _bytes[size++] = (byte) (0x80 | (v >>> 14));
265+ case 1:
266+ _bytes[size++] = (byte) (0x80 | (v >>> 7));
267+ case 2:
268+ _bytes[size++] = (byte) (0x80 | v);
269+ }
270+
271+ return endSegment(size);
272+ } catch (final ArrayIndexOutOfBoundsException e) {
273+ return tooLong(save);
274+ }
275 }
276
277 /**
278@@ -1735,11 +1759,14 @@
279 * @return This <code>Key</code>, to permit method call chaining
280 */
281 public Key append(final int v) {
282- testValidForAppend();
283- final int size = appendIntInternal(v);
284- // Close out the segment.
285-
286- return endSegment(size);
287+ final int save = _size;
288+ try {
289+ testValidForAppend();
290+ final int size = appendIntInternal(v);
291+ return endSegment(size);
292+ } catch (final ArrayIndexOutOfBoundsException e) {
293+ return tooLong(save);
294+ }
295 }
296
297 /**
298@@ -1821,9 +1848,14 @@
299 * @return This <code>Key</code>, to permit method call chaining
300 */
301 public Key append(final long v) {
302- testValidForAppend();
303- final int size = appendLongInternal(v);
304- return endSegment(size);
305+ final int save = _size;
306+ try {
307+ testValidForAppend();
308+ final int size = appendLongInternal(v);
309+ return endSegment(size);
310+ } catch (final ArrayIndexOutOfBoundsException e) {
311+ return tooLong(save);
312+ }
313 }
314
315 private int appendLongInternal(final long v) {
316@@ -1921,21 +1953,26 @@
317 * @return This <code>Key</code>, to permit method call chaining
318 */
319 public Key append(final float v) {
320- testValidForAppend();
321- int bits = Float.floatToIntBits(v);
322- int size = _size;
323- _bytes[size++] = TYPE_FLOAT;
324- if (bits < 0) {
325- bits = ~bits;
326- } else {
327- bits ^= 0x80000000;
328- }
329- while (bits != 0) {
330- _bytes[size++] = (byte) (0x80 | (bits >> 25));
331- bits <<= 7;
332- }
333+ final int save = _size;
334+ try {
335+ testValidForAppend();
336+ int bits = Float.floatToIntBits(v);
337+ int size = _size;
338+ _bytes[size++] = TYPE_FLOAT;
339+ if (bits < 0) {
340+ bits = ~bits;
341+ } else {
342+ bits ^= 0x80000000;
343+ }
344+ while (bits != 0) {
345+ _bytes[size++] = (byte) (0x80 | (bits >> 25));
346+ bits <<= 7;
347+ }
348
349- return endSegment(size);
350+ return endSegment(size);
351+ } catch (final ArrayIndexOutOfBoundsException e) {
352+ return tooLong(save);
353+ }
354 }
355
356 /**
357@@ -1946,21 +1983,26 @@
358 * @return This <code>Key</code>, to permit method call chaining
359 */
360 public Key append(final double v) {
361- testValidForAppend();
362- long bits = Double.doubleToLongBits(v);
363- int size = _size;
364- _bytes[size++] = TYPE_DOUBLE;
365- if (bits < 0) {
366- bits = ~bits;
367- } else {
368- bits ^= 0x8000000000000000L;
369- }
370- while (bits != 0) {
371- _bytes[size++] = (byte) (0x80 | (bits >> 57));
372- bits <<= 7;
373- }
374+ final int save = _size;
375+ try {
376+ testValidForAppend();
377+ long bits = Double.doubleToLongBits(v);
378+ int size = _size;
379+ _bytes[size++] = TYPE_DOUBLE;
380+ if (bits < 0) {
381+ bits = ~bits;
382+ } else {
383+ bits ^= 0x8000000000000000L;
384+ }
385+ while (bits != 0) {
386+ _bytes[size++] = (byte) (0x80 | (bits >> 57));
387+ bits <<= 7;
388+ }
389
390- return endSegment(size);
391+ return endSegment(size);
392+ } catch (final ArrayIndexOutOfBoundsException e) {
393+ return tooLong(save);
394+ }
395 }
396
397 /**
398@@ -2072,6 +2114,7 @@
399 }
400
401 throw new ConversionException("Object class " + object.getClass().getName() + " can't be used in a Key");
402+
403 }
404
405 /**
406@@ -2081,16 +2124,22 @@
407 *
408 * @param key
409 */
410- public void appendKeySegment(final Key key) {
411- int length = 0;
412- for (int index = key.getIndex(); index < key.getEncodedSize(); index++) {
413- length++;
414- if (key.getEncodedBytes()[index] == 0) {
415- break;
416+ public Key appendKeySegment(final Key key) {
417+ final int save = _size;
418+ try {
419+ int length = 0;
420+ for (int index = key.getIndex(); index < key.getEncodedSize(); index++) {
421+ length++;
422+ if (key.getEncodedBytes()[index] == 0) {
423+ length--;
424+ break;
425+ }
426 }
427+ System.arraycopy(key.getEncodedBytes(), key.getIndex(), _bytes, _size, length);
428+ return endSegment(_size + length);
429+ } catch (final ArrayIndexOutOfBoundsException e) {
430+ return tooLong(save);
431 }
432- System.arraycopy(key.getEncodedBytes(), key.getIndex(), _bytes, _size, length);
433- _size += length;
434 }
435
436 /**
437@@ -3325,33 +3374,34 @@
438 * @return This <code>Key</code>, to permit method call chaining
439 */
440 private Key appendString(final CharSequence s, final CoderContext context) {
441- notLeftOrRightGuard();
442- testValidForAppend();
443- final int strlen = s.length();
444- if (strlen > _maxSize) {
445- throw new ConversionException("Requested size=" + strlen + " exceeds maximum size=" + _maxSize);
446- }
447- int size = _size;
448- _bytes[size++] = (byte) TYPE_STRING;
449+ final int save = _size;
450+ try {
451+ notLeftOrRightGuard();
452+ testValidForAppend();
453+ final int strlen = s.length();
454+ int size = _size;
455+ _bytes[size++] = (byte) TYPE_STRING;
456
457- for (int i = 0; i < strlen; i++) {
458- final int c = s.charAt(i);
459- if (c <= 0x0001) {
460- _bytes[size++] = (byte) (0x01);
461- _bytes[size++] = (byte) (c + 0x0020);
462- } else if (c <= 0x007F) {
463- _bytes[size++] = (byte) c;
464- } else if (c <= 0x07FF) {
465- _bytes[size++] = (byte) (0xC0 | ((c >> 6) & 0x1F));
466- _bytes[size++] = (byte) (0x80 | ((c >> 0) & 0x3F));
467- } else {
468- _bytes[size++] = (byte) (0xE0 | ((c >> 12) & 0x0F));
469- _bytes[size++] = (byte) (0x80 | ((c >> 6) & 0x3F));
470- _bytes[size++] = (byte) (0x80 | ((c >> 0) & 0x3F));
471+ for (int i = 0; i < strlen; i++) {
472+ final int c = s.charAt(i);
473+ if (c <= 0x0001) {
474+ _bytes[size++] = (byte) (0x01);
475+ _bytes[size++] = (byte) (c + 0x0020);
476+ } else if (c <= 0x007F) {
477+ _bytes[size++] = (byte) c;
478+ } else if (c <= 0x07FF) {
479+ _bytes[size++] = (byte) (0xC0 | ((c >> 6) & 0x1F));
480+ _bytes[size++] = (byte) (0x80 | ((c >> 0) & 0x3F));
481+ } else {
482+ _bytes[size++] = (byte) (0xE0 | ((c >> 12) & 0x0F));
483+ _bytes[size++] = (byte) (0x80 | ((c >> 6) & 0x3F));
484+ _bytes[size++] = (byte) (0x80 | ((c >> 0) & 0x3F));
485+ }
486 }
487+ return endSegment(size);
488+ } catch (final ArrayIndexOutOfBoundsException e) {
489+ return tooLong(save);
490 }
491-
492- return endSegment(size);
493 }
494
495 /**
496@@ -3360,9 +3410,14 @@
497 * @return This <code>Key</code>, to permit method call chaining
498 */
499 private Key appendNull() {
500- int size = _size;
501- _bytes[size++] = TYPE_NULL;
502- return endSegment(size);
503+ final int save = _size;
504+ try {
505+ int size = _size;
506+ _bytes[size++] = TYPE_NULL;
507+ return endSegment(size);
508+ } catch (final ArrayIndexOutOfBoundsException e) {
509+ return tooLong(save);
510+ }
511 }
512
513 private Key appendByKeyCoder(final Object object, final Class<?> cl, final KeyCoder coder,
514@@ -3379,6 +3434,8 @@
515 endSegment(_size);
516 size = _size;
517 return this;
518+ } catch (final ArrayIndexOutOfBoundsException e) {
519+ return tooLong(size);
520 } finally {
521 _size = size;
522 _inKeyCoder = saveInKeyCoder;
523@@ -3480,14 +3537,19 @@
524 * level. The result key value should only be used in traversal operations.
525 */
526 Key appendBefore() {
527- int size = _size;
528- _bytes[size++] = TYPE_BEFORE;
529- _bytes[size] = 0;
530- _size = size;
531- if (_depth != -1)
532- _depth++;
533- bumpGeneration();
534- return this;
535+ final int save = _size;
536+ try {
537+ int size = _size;
538+ _bytes[size++] = TYPE_BEFORE;
539+ _bytes[size] = 0;
540+ _size = size;
541+ if (_depth != -1)
542+ _depth++;
543+ bumpGeneration();
544+ return this;
545+ } catch (final ArrayIndexOutOfBoundsException e) {
546+ return tooLong(save);
547+ }
548 }
549
550 /**
551@@ -3496,34 +3558,54 @@
552 * operations.
553 */
554 Key appendAfter() {
555- int size = _size;
556- _bytes[size++] = (byte) TYPE_AFTER;
557- _bytes[size] = 0;
558- _size = size;
559- if (_depth != -1)
560- _depth++;
561- bumpGeneration();
562- return this;
563+ final int save = _size;
564+ try {
565+ int size = _size;
566+ _bytes[size++] = (byte) TYPE_AFTER;
567+ _bytes[size] = 0;
568+ _size = size;
569+ if (_depth != -1)
570+ _depth++;
571+ bumpGeneration();
572+ return this;
573+ } catch (final ArrayIndexOutOfBoundsException e) {
574+ return tooLong(save);
575+ }
576 }
577
578 private Key appendDate(final Date v) {
579- _bytes[_size++] = (byte) TYPE_DATE;
580- final int size = appendLongInternal(v.getTime());
581- return endSegment(size);
582+ final int save = _size;
583+ try {
584+ _bytes[_size++] = (byte) TYPE_DATE;
585+ final int size = appendLongInternal(v.getTime());
586+ return endSegment(size);
587+ } catch (final ArrayIndexOutOfBoundsException e) {
588+ return tooLong(save);
589+ }
590 }
591
592 private Key appendBigInteger(final BigInteger v) {
593- _bytes[_size++] = TYPE_BIG_INTEGER;
594- appendBigInteger(v, 0);
595- endSegment(_size);
596- return this;
597+ final int save = _size;
598+ try {
599+ _bytes[_size++] = TYPE_BIG_INTEGER;
600+ appendBigInteger(v, 0);
601+ endSegment(_size);
602+ return this;
603+ } catch (final ArrayIndexOutOfBoundsException e) {
604+ return tooLong(save);
605+ }
606 }
607
608 private Key appendBigDecimal(final BigDecimal v) {
609- _bytes[_size++] = TYPE_BIG_DECIMAL;
610- appendBigInteger(v.unscaledValue(), v.scale());
611- endSegment(_size);
612- return this;
613+ final int save = _size;
614+ try {
615+ _bytes[_size++] = TYPE_BIG_DECIMAL;
616+ appendBigInteger(v.unscaledValue(), v.scale());
617+ endSegment(_size);
618+ return this;
619+ } catch (final ArrayIndexOutOfBoundsException e) {
620+ return tooLong(save);
621+ }
622 }
623
624 /**
625@@ -3541,12 +3623,17 @@
626 * @return This <code>Key</code>, to permit method call chaining
627 */
628 public Key appendByteArray(final byte[] bytes, final int offset, final int size) {
629- _bytes[_size++] = TYPE_BYTE_ARRAY;
630- int keySize = _size;
631- System.arraycopy(bytes, offset, _bytes, keySize, size);
632- _size += size;
633- keySize += quoteNulls(keySize, size, false);
634- return endSegment(keySize);
635+ final int save = _size;
636+ try {
637+ _bytes[_size++] = TYPE_BYTE_ARRAY;
638+ int keySize = _size;
639+ System.arraycopy(bytes, offset, _bytes, keySize, size);
640+ _size += size;
641+ keySize += quoteNulls(keySize, size, false);
642+ return endSegment(keySize);
643+ } catch (final ArrayIndexOutOfBoundsException e) {
644+ return tooLong(save);
645+ }
646 }
647
648 private int getTypeCode() {
649@@ -3622,6 +3709,11 @@
650 return this;
651 }
652
653+ private Key tooLong(final int originalSize) throws KeyTooLongException {
654+ _size = originalSize;
655+ throw new KeyTooLongException("Maximum size=" + _maxSize + " original size=" + originalSize);
656+ }
657+
658 private Key setRightEdge() {
659 _bytes[0] = (byte) 255;
660 _size = 1;
661
662=== modified file 'src/main/java/com/persistit/Persistit.java'
663--- src/main/java/com/persistit/Persistit.java 2013-03-13 15:55:19 +0000
664+++ src/main/java/com/persistit/Persistit.java 2013-03-20 19:33:22 +0000
665@@ -1691,7 +1691,7 @@
666 releaseAllResources();
667 }
668
669- private void closeZombieTransactions(boolean removeAllSessions) {
670+ private void closeZombieTransactions(final boolean removeAllSessions) {
671 final Set<SessionId> sessionIds;
672 synchronized (_transactionSessionMap) {
673 sessionIds = new HashSet<SessionId>(_transactionSessionMap.keySet());
674
675=== modified file 'src/main/java/com/persistit/Transaction.java'
676--- src/main/java/com/persistit/Transaction.java 2013-03-13 16:27:04 +0000
677+++ src/main/java/com/persistit/Transaction.java 2013-03-20 19:33:22 +0000
678@@ -685,7 +685,7 @@
679 }
680 try {
681 pruneLockPages();
682- } catch (Exception e) {
683+ } catch (final Exception e) {
684 _persistit.getLogBase().pruneException.log(e, "locks");
685 }
686 _transactionStatus = null;
687
688=== added file 'src/main/java/com/persistit/exception/KeyTooLongException.java'
689--- src/main/java/com/persistit/exception/KeyTooLongException.java 1970-01-01 00:00:00 +0000
690+++ src/main/java/com/persistit/exception/KeyTooLongException.java 2013-03-20 19:33:22 +0000
691@@ -0,0 +1,39 @@
692+/**
693+ * Copyright © 2005-2012 Akiban Technologies, Inc. All rights reserved.
694+ *
695+ * This program and the accompanying materials are made available
696+ * under the terms of the Eclipse Public License v1.0 which
697+ * accompanies this distribution, and is available at
698+ * http://www.eclipse.org/legal/epl-v10.html
699+ *
700+ * This program may also be available under different license terms.
701+ * For more information, see www.akiban.com or contact licensing@akiban.com.
702+ *
703+ * Contributors:
704+ * Akiban Technologies, Inc.
705+ */
706+
707+package com.persistit.exception;
708+
709+/**
710+ * Thrown by {@link com.persistit.Key} when an attempt to append a key segment
711+ * exceeds the maximum size of the key.
712+ *
713+ * @version 1.0
714+ */
715+public class KeyTooLongException extends RuntimeException {
716+
717+ private static final long serialVersionUID = -4657691754665822537L;
718+
719+ public KeyTooLongException() {
720+ super();
721+ }
722+
723+ public KeyTooLongException(final String msg) {
724+ super(msg);
725+ }
726+
727+ public KeyTooLongException(final Throwable t) {
728+ super(t);
729+ }
730+}
731
732=== modified file 'src/test/java/com/persistit/TransactionAbandonedTest.java'
733--- src/test/java/com/persistit/TransactionAbandonedTest.java 2013-02-20 17:13:17 +0000
734+++ src/test/java/com/persistit/TransactionAbandonedTest.java 2013-03-20 19:33:22 +0000
735@@ -15,13 +15,14 @@
736
737 package com.persistit;
738
739+import static org.junit.Assert.assertEquals;
740+
741+import org.junit.Before;
742+import org.junit.Test;
743+
744 import com.persistit.exception.PersistitException;
745 import com.persistit.unit.ConcurrentUtil;
746 import com.persistit.unit.UnitTestProperties;
747-import org.junit.Before;
748-import org.junit.Test;
749-
750-import static org.junit.Assert.assertEquals;
751
752 /**
753 * <p>
754@@ -51,7 +52,7 @@
755 private final boolean doRead;
756 private final boolean doWrite;
757
758- public TxnAbandoner(Persistit persistit, boolean doRead, boolean doWrite) {
759+ public TxnAbandoner(final Persistit persistit, final boolean doRead, final boolean doWrite) {
760 this.persistit = persistit;
761 this.doRead = doRead;
762 this.doWrite = doWrite;
763@@ -59,7 +60,7 @@
764
765 @Override
766 public void run() throws PersistitException {
767- Transaction txn = persistit.getTransaction();
768+ final Transaction txn = persistit.getTransaction();
769 txn.begin();
770 if (doRead) {
771 assertEquals("Traverse count", KEY_RANGE, scanAndCount(getExchange(persistit)));
772@@ -70,18 +71,19 @@
773 }
774 }
775
776- private static Exchange getExchange(Persistit persistit) throws PersistitException {
777+ private static Exchange getExchange(final Persistit persistit) throws PersistitException {
778 return persistit.getExchange(UnitTestProperties.VOLUME_NAME, TREE, true);
779 }
780
781- private static void loadData(Persistit persistit, int keyOffset, int count) throws PersistitException {
782- Exchange ex = getExchange(persistit);
783+ private static void loadData(final Persistit persistit, final int keyOffset, final int count)
784+ throws PersistitException {
785+ final Exchange ex = getExchange(persistit);
786 for (int i = 0; i < count; ++i) {
787 ex.clear().append(keyOffset + i).store();
788 }
789 }
790
791- private static int scanAndCount(Exchange ex) throws PersistitException {
792+ private static int scanAndCount(final Exchange ex) throws PersistitException {
793 ex.clear().append(Key.BEFORE);
794 int saw = 0;
795 while (ex.next()) {
796@@ -96,8 +98,8 @@
797 loadData(_persistit, KEY_START, KEY_RANGE);
798 }
799
800- private void runAndCleanup(String name, boolean doRead, boolean doWrite) {
801- Thread t = ConcurrentUtil.createThread(name, new TxnAbandoner(_persistit, false, false));
802+ private void runAndCleanup(final String name, final boolean doRead, final boolean doWrite) {
803+ final Thread t = ConcurrentUtil.createThread(name, new TxnAbandoner(_persistit, false, false));
804 ConcurrentUtil.startAndJoinAssertSuccess(MAX_TIMEOUT_MS, t);
805 // Threw exception before fix
806 _persistit.cleanup();
807@@ -119,11 +121,11 @@
808 runAndCleanup("ReadAndWrite", true, true);
809 assertEquals("Traversed after abandoned", KEY_RANGE, scanAndCount(getExchange(_persistit)));
810 // Check that the abandoned was pruned
811- CleanupManager cm = _persistit.getCleanupManager();
812+ final CleanupManager cm = _persistit.getCleanupManager();
813 for (int i = 0; i < 5 && cm.getEnqueuedCount() > 0; ++i) {
814 cm.runTask();
815 }
816- Exchange rawEx = getExchange(_persistit);
817+ final Exchange rawEx = getExchange(_persistit);
818 rawEx.ignoreMVCCFetch(true);
819 assertEquals("Raw traversed after abandoned", KEY_RANGE, scanAndCount(rawEx));
820 }
821
822=== modified file 'src/test/java/com/persistit/unit/ExchangeTest.java'
823--- src/test/java/com/persistit/unit/ExchangeTest.java 2012-12-24 18:27:56 +0000
824+++ src/test/java/com/persistit/unit/ExchangeTest.java 2013-03-20 19:33:22 +0000
825@@ -31,7 +31,7 @@
826 import com.persistit.PersistitUnitTestCase;
827 import com.persistit.Transaction;
828 import com.persistit.Volume;
829-import com.persistit.exception.ConversionException;
830+import com.persistit.exception.KeyTooLongException;
831 import com.persistit.exception.PersistitException;
832
833 public class ExchangeTest extends PersistitUnitTestCase {
834@@ -151,8 +151,8 @@
835 randomKey = createString(initialLength);
836 try {
837 ex.clear().append(randomKey);
838- fail("ConversionException should have been thrown");
839- } catch (final ConversionException expected) {
840+ fail("KeyTooLongException should have been thrown");
841+ } catch (final KeyTooLongException expected) {
842 }
843 }
844
845
846=== modified file 'src/test/java/com/persistit/unit/KeyTest1.java'
847--- src/test/java/com/persistit/unit/KeyTest1.java 2012-11-07 14:03:51 +0000
848+++ src/test/java/com/persistit/unit/KeyTest1.java 2013-03-20 19:33:22 +0000
849@@ -22,6 +22,7 @@
850
851 import java.math.BigDecimal;
852 import java.math.BigInteger;
853+import java.util.Date;
854
855 import junit.framework.Assert;
856
857@@ -33,8 +34,8 @@
858 import com.persistit.PersistitUnitTestCase;
859 import com.persistit.TestShim;
860 import com.persistit.exception.InvalidKeyException;
861+import com.persistit.exception.KeyTooLongException;
862 import com.persistit.exception.MissingKeySegmentException;
863-import com.persistit.util.Util;
864
865 public class KeyTest1 extends PersistitUnitTestCase {
866
867@@ -45,22 +46,26 @@
868 private double dv1;
869 private double dv2;
870
871- long[] TEST_LONGS = { 0, 1, 2, 3, 123, 126, 127, 128, 129, 130, 131, 132, 250, 251, 252, 253, 254, 255, 256, 257,
872- 258, 259, 260, 4094, 4095, 4096, 4097, 4098, 16383, 16384, 16385, 0x1FFFFEL, 0x1FFFFFL, 0x200000L,
873- 0x3FFFFFFFFFEL, 0x3FFFFFFFFFFL, 0x2000000000000L, 0x1FFFFFFFFFFFFL, 0x1FFFFFFFFFFFEL,
874+ private final static long[] TEST_LONGS = { 0, 1, 2, 3, 123, 126, 127, 128, 129, 130, 131, 132, 250, 251, 252, 253,
875+ 254, 255, 256, 257, 258, 259, 260, 4094, 4095, 4096, 4097, 4098, 16383, 16384, 16385, 0x1FFFFEL, 0x1FFFFFL,
876+ 0x200000L, 0x3FFFFFFFFFEL, 0x3FFFFFFFFFFL, 0x2000000000000L, 0x1FFFFFFFFFFFFL, 0x1FFFFFFFFFFFEL,
877 Integer.MAX_VALUE - 2, Integer.MAX_VALUE - 1, Integer.MAX_VALUE + 0, Integer.MAX_VALUE + 1,
878 Integer.MAX_VALUE + 2, Integer.MAX_VALUE + 3, Long.MIN_VALUE, Long.MIN_VALUE + 1, Long.MIN_VALUE + 2,
879 Long.MAX_VALUE, Long.MAX_VALUE - 1, Long.MAX_VALUE - 2, };
880
881- float[] TEST_FLOATS = { 0.0F, 1.0F, 2.0F, 12345.0F, 0.0003F, 1.2345E-10F, 1.12345E-20F, 0.0F, -1.0F, -2.0F,
882- -12345.0F, -0.0003F, -1.2345E-10F, -1.12345E-20F, Float.MAX_VALUE, Float.MIN_VALUE, Float.MAX_VALUE / 2.0F,
883- Float.MAX_VALUE / 3.0F, Float.MAX_VALUE / 4.0F, Float.MIN_VALUE / 2.0F, Float.MIN_VALUE / 3.0F,
884- Float.MIN_VALUE / 4.0F, Float.NaN, Float.NEGATIVE_INFINITY, Float.POSITIVE_INFINITY };
885-
886- double[] TEST_DOUBLES = { 0.0, 1.0, 2.0, 12345.0, 0.0003, 1.2345E-10, 1.12345E-20, 0.0, -1.0, -2.0, -12345.0,
887- -0.0003, -1.2345E-10, -1.12345E-20, Double.MAX_VALUE, Double.MIN_VALUE, Double.MAX_VALUE / 2.0,
888- Double.MAX_VALUE / 3.0, Double.MAX_VALUE / 4.0, Double.MIN_VALUE / 2.0, Double.MIN_VALUE / 3.0,
889- Double.MIN_VALUE / 4.0, Double.NaN, Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY };
890+ private final static float[] TEST_FLOATS = { 0.0F, 1.0F, 2.0F, 12345.0F, 0.0003F, 1.2345E-10F, 1.12345E-20F, 0.0F,
891+ -1.0F, -2.0F, -12345.0F, -0.0003F, -1.2345E-10F, -1.12345E-20F, Float.MAX_VALUE, Float.MIN_VALUE,
892+ Float.MAX_VALUE / 2.0F, Float.MAX_VALUE / 3.0F, Float.MAX_VALUE / 4.0F, Float.MIN_VALUE / 2.0F,
893+ Float.MIN_VALUE / 3.0F, Float.MIN_VALUE / 4.0F, Float.NaN, Float.NEGATIVE_INFINITY, Float.POSITIVE_INFINITY };
894+
895+ private final static double[] TEST_DOUBLES = { 0.0, 1.0, 2.0, 12345.0, 0.0003, 1.2345E-10, 1.12345E-20, 0.0, -1.0,
896+ -2.0, -12345.0, -0.0003, -1.2345E-10, -1.12345E-20, Double.MAX_VALUE, Double.MIN_VALUE,
897+ Double.MAX_VALUE / 2.0, Double.MAX_VALUE / 3.0, Double.MAX_VALUE / 4.0, Double.MIN_VALUE / 2.0,
898+ Double.MIN_VALUE / 3.0, Double.MIN_VALUE / 4.0, Double.NaN, Double.NEGATIVE_INFINITY,
899+ Double.POSITIVE_INFINITY };
900+
901+ private final static Object[] TEST_OBJECTS = new Object[] { null, true, false, (byte) 1, (char) 2, (short) 3, 4,
902+ (long) 5, 6.0f, 7.0d, "This is a String", new Date(), new BigInteger("1"), new BigDecimal("2.2") };
903
904 @Test
905 public void test1() {
906@@ -1013,6 +1018,24 @@
907 assertEquals("Key value incorrect", "{1,2,3,\"abc\",\"abc\"}", key1.toString());
908 }
909
910+ @Test
911+ public void testKeyTooLong() throws Exception {
912+ for (int maxSize = 1; maxSize < 99; maxSize++) {
913+ mainLoop: for (final Object value : TEST_OBJECTS) {
914+ final Key key = new Key(_persistit, maxSize);
915+ // Every appended value consumes at least 2 bytes
916+ for (int i = 0; i < 50; i++) {
917+ try {
918+ key.append(value);
919+ } catch (final KeyTooLongException e) {
920+ continue mainLoop;
921+ }
922+ }
923+ fail("Should have thrown a KeyTooLongException for maxSize=" + maxSize + " and value " + value);
924+ }
925+ }
926+ }
927+
928 private static boolean doubleEquals(final double f1, final double f2) {
929 if (Double.isNaN(f1)) {
930 return Double.isNaN(f2);
931@@ -1023,45 +1046,6 @@
932 return f1 == f2;
933 }
934
935- @Override
936- public void runAllTests() throws Exception {
937- test1();
938- test2();
939- test3();
940- test4();
941- test5();
942- test6();
943- test7();
944- test8();
945- test9();
946- test10();
947- test11();
948- test12();
949- test13();
950- test14();
951- }
952-
953- private String floatBits(final float v) {
954- final int bits = Float.floatToIntBits(v);
955- final StringBuilder sb = new StringBuilder();
956- Util.hex(sb, bits, 8);
957- return sb.toString();
958- }
959-
960- private String doubleBits(final double v) {
961- final long bits = Double.doubleToLongBits(v);
962- final StringBuilder sb = new StringBuilder();
963- Util.hex(sb, bits, 16);
964- return sb.toString();
965- }
966-
967- private void debug(final boolean condition) {
968- if (!condition) {
969- return;
970- }
971- return; // <-- breakpoint here
972- }
973-
974 private Key newKey() {
975 return new Key(_persistit);
976 }

Subscribers

People subscribed via source and target branches