Re: [Pki-devel] Review Request

Christina, thanks for the review, comments below: ----- Original Message ----- From: "Christina Fu" <cfu(a)redhat.com&gt; To: pki-devel(a)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: _______________________________________________ Pki-devel mailing list Pki-devel(a)redhat.com https://www.redhat.com/mailman/listinfo/pki-devel