[Pki-devel] Review Request

[John Magne]

Christina, thanks for the review, comments below:



----- Original Message -----
From: "Christina Fu" <cfu at redhat.com>
To: pki-devel at redhat.com
Sent: Tuesday, October 23, 2012 9:53:33 AM
Subject: Re: [Pki-devel] Review Request

The following are my review comments:

- I see you replaced the PL_strcat() calls in mod_tokendb.cpp.  How 
about the other PL_strcat() calls in other files? Do they not present 
any issue?

Jack: This particular problem was isolated to mod_tokendb.cpp. It made sense to concentrate on the specific problem this round and deal with the rest later.

- The following RA::Debug() message and comment seems a bit confusing 
for me as it seems to declare that it's about to truncate and yet it's 
actually trying to resize:



A::Debug( "safe_injection_strcat, about to truncate, resize injection 
buffer:  ", "current len: %d expected_len %d data_len: %d 
cur_injection_size %d",current_len, expected_len, cat_data_len, 
*injection_size );
/* We are going to get truncated!

How about say something like "...resizing to avoid truncation..."

- safe_injection_strcat()

Jack: Will take care of it, thanks.



not sure of the role of the variable "result" as it only gets set once 
at the end, but returned many times in the middle. Unclear if 1 is 
success or 0 is. There is no comment (yeah, could you please provide 
comment for parameters and return value?). It is especially confusing 
when check_injection_size() comment says "returns 0 on success, 1 on 
failure" and yet safe_injection_strcat() has "if (check_res == 1), 
return result", where result was init'd to be 0,  So does that mean it 
is the opposite of the check_injection_size()?
Another thing is the callers ignore the return values from 
safe_injection_strcat() anyway, so why return anything at all?



Jack: Good catch, there is not much we can do if this routine fails. Also, the worst result now is truncation.



- Why only call check_injection_size 2nd time if still not enough?

Jack: This was done for safety, there is a small chance that the first realloc would not cover the data being added. It seemed prudent to try one more time in such a rare case. In case it fails, truncation would be the only bad result. 

Christina

On 10/22/2012 02:54 PM, John Magne wrote:
> Request review for issue:
>
>
> Bug 864607 - Empty certificate search in TPS results in httpd.worker segmentation fault then server error.
>
> Bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=864607
>
> Attachment:
>
> https://bugzilla.redhat.com/attachment.cgi?id=630239&action=diff
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________
Pki-devel mailing list
Pki-devel at redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel