Check index bounds in compact protocol reader. #16493
Conversation
| @@ -399,6 +404,7 @@ struct parquet_field_binary_list | |||
| auto const l = cpr->get_u32(); | |||
| CUDF_EXPECTS(l <= static_cast<size_t>(cpr->m_end - cpr->m_cur), "binary length mismatch"); | |||
|
|
|||
| CUDF_EXPECTS(i >= 0 and i < val.size()); | |||
There was a problem hiding this comment.
Looks like the vector is resized in the next line so maybe this check should include l instead of val.size()
Sorry, the resize is on the vector's vector.
There was a problem hiding this comment.
From L103-105, the parquet_field_list base class where these functions are being called from, it does look like we have an assert which checks the requested field's size, and then modifies i and v appropriately to pass to the sub-functions so the checks do seem redundant. I am okay either way though, especially if it was flagged by a security scan.
There was a problem hiding this comment.
At L103-105, the
parquet_field_listbase class assert checks the requested field's type and adjustv.size()and i appropriately before passing them to these sub-functions so the added checks do seem redundant to me.
I am okay with the checks if needed to pass security checks. Otherwise, we can maybe suppress them.
|
@mhaseeb123 I agree it would be good to suppress these checks. I don't know how these checks work but I will inquire. For now, I think we're safer just to enforce the requirement directly. |
|
/merge |
Description
This adds bounds checking to the compact protocol reader's read function.
Checklist