superreview requested: [Bug 808217] Sporadic failure of 'certutil' to convert ASCII cert request to binary . . . : [Attachment 739967] Additional patch v4, add and use SECITEM_ReallocItemV2

[bugzill...@mozilla.org]

Kai Engert (:kaie) <ka...@kuix.de> has asked Wan-Teh Chang <w...@google.com> for
superreview:
Bug 808217: Sporadic failure of 'certutil' to convert ASCII cert request to
binary . . .
https://bugzilla.mozilla.org/show_bug.cgi?id=808217

Attachment 739967: Additional patch v4, add and use SECITEM_ReallocItemV2
https://bugzilla.mozilla.org/attachment.cgi?id=739967&action=edit


------- Additional Comments from Kai Engert (:kaie) <ka...@kuix.de>
I think the new API doesn't need the "oldlen" parameter.

I've copied and simplified the old implementation, and fixed the bugs that were
mentioned in bug 298649 and bug 298938.

[bugzill...@mozilla.org]

Wan-Teh Chang <w...@google.com> has not granted Kai Engert (:kaie)
<ka...@kuix.de>'s request for superreview:

Bug 808217: Sporadic failure of 'certutil' to convert ASCII cert request to
binary . . .
https://bugzilla.mozilla.org/show_bug.cgi?id=808217

Attachment 739967: Additional patch v4, add and use SECITEM_ReallocItemV2
https://bugzilla.mozilla.org/attachment.cgi?id=739967&action=edit

------- Additional Comments from Wan-Teh Chang <w...@google.com>
Review of attachment 739967:
-----------------------------------------------------------------

Kai, this patch looks good except for one problem. I suggest a fix for that
problem below.

::: lib/util/secitem.c
@@ +130,5 @@
> + }
> +
> + if (item->len >= newlen) {
> + /* There's no need to realloc, and no need to potentially
> + * fail on realloc, because the buffer already has sufficient

It is not a problem for realloc() to fail as long as this function
doesn't modify |item|.

I think it can be useful to shrink item->data, so we should
still call realloc() in that case.

However, what you're doing here is apppropriate when |arena| is
non-NULL. So I suggest we rewrite this if statement as follows:

if (arena && item->len >= newlen) {
/* There's no need to realloc a shorter block from the arena
* because we would simply use more memory.
*/
item->len = newlen;
return SECSuccess;
}

@@ +133,5 @@
> + /* There's no need to realloc, and no need to potentially
> + * fail on realloc, because the buffer already has sufficient
> + * size. We don't want to store the shorter len in the item,
> + * because that might confuse a later call to PORT_ArenaGrow on
> + * the same item? Therefore, we'll simply keep everything as is,

We must set item->len to newlen whenever this function returns
SECSuccess.

@@ +135,5 @@
> + * size. We don't want to store the shorter len in the item,
> + * because that might confuse a later call to PORT_ArenaGrow on
> + * the same item? Therefore, we'll simply keep everything as is,
> + * and return success.
> + * This includes the case if newlen is zero.

If newlen is zero, realloc() acts like free(). So I think
SECITEM_ReallocItemV2() probably should free item->data in
that case.

::: lib/util/secitem.h
@@ +37,5 @@
> extern SECItem *SECITEM_AllocItem(PLArenaPool *arena, SECItem *item,
> unsigned int len);
>
> /*
> +** This is a legacy function cotaining bugs. However, the function is

Typo: cotaining => containing

We should be clear which bugs we are not fixing. I believe the only
bug that we cannot fix is the failure to update item->len.

@@ +48,5 @@
> ** In any case, "item" is expected to be a valid SECItem pointer;
> ** SECFailure is returned if it is not. If the allocation succeeds,
> ** SECSuccess is returned.
> */
> +extern SECStatus SECITEM_ReallocItem( /* deprecated */

Please remove this /* deprecated */ comment.

Putting this comment in the function's argument list is confusing
because it seems to say only the first argument |arena| is deprecated.

[bugzill...@mozilla.org]

Kai Engert (:kaie) <ka...@kuix.de> has asked Wan-Teh Chang <w...@google.com> for

superreview:
Bug 808217: Sporadic failure of 'certutil' to convert ASCII cert request to
binary . . .
https://bugzilla.mozilla.org/show_bug.cgi?id=808217

Attachment 743662: additional patch v5
https://bugzilla.mozilla.org/attachment.cgi?id=743662&action=edit

[bugzill...@mozilla.org]

Kai Engert (:kaie) <ka...@kuix.de> has canceled Kai Engert (:kaie)
<ka...@kuix.de>'s request for superreview: