Conversation
|
@opencv-alalek @asmorkalov care to review this one too? The If you are happy with the technique, I will finish the conversion. |
| int cn = (elem_type == CV_SEQ_ELTYPE_PTR/*CV_USRTYPE1*/) ? 1 : CV_MAT_CN(elem_type); | ||
| char symbol = (elem_type == CV_SEQ_ELTYPE_PTR/*CV_USRTYPE1*/) ? 'r' : typeSymbol(CV_MAT_DEPTH(elem_type)); | ||
| sprintf(dt, "%d%c", cn, symbol); | ||
| snprintf(dt, dt_len, "%d%c", cn, symbol); |
There was a problem hiding this comment.
I believe we need to check the return code of snprintf call (as we pass dt_len):
a return value of size or more means that the output was truncated.
CV_CheckGE(snprintf_result, 0, ""); // check for snprintf errors
CV_CheckLT((size_t)snprintf_result, dt_len, "output is truncated");
There was a problem hiding this comment.
But dt and dt_len are both given by the caller. If truncation occurs, it's because:
- the caller passed the wrong size for
dt_len - truncation was desired
The callers do the correct thing, one example:
fs << "dt" << fs::encodeFormat( m.type(), dt, sizeof(dt) );
| tmp += snprintf( buffer + tmp, bufsize - tmp, "WIDTH %d\n", width); | ||
| tmp += snprintf( buffer + tmp, bufsize - tmp, "HEIGHT %d\n", height); | ||
| tmp += snprintf( buffer + tmp, bufsize - tmp, "DEPTH %d\n", img.channels()); | ||
| tmp += snprintf( buffer + tmp, bufsize - tmp, "MAXVAL %d\n", (1 << img.elemSize1()*8) - 1); |
There was a problem hiding this comment.
It is better to use std::string / std::ostringstream (preferable) instead (refactor code).
snprintf could return negative values (in general, but not in this case) and they are not handled properly.
| int header_sz = sprintf(buffer, "P%c\n%d %d\n", | ||
| int header_sz = snprintf(buffer, bufferSize, "P%c\n%d %d\n", | ||
| (char)('0' + code), width, height); | ||
| CV_Assert(header_sz > 0); |
There was a problem hiding this comment.
bufferSize < bufferSize should be checked too.
|
|
||
| CV_EXPORTS const char* convertTypeStr(int sdepth, int ddepth, int cn, char* buf); | ||
| CV_EXPORTS CV_DEPRECATED const char* convertTypeStr(int sdepth, int ddepth, int cn, char* buf); | ||
| CV_EXPORTS String convertTypeStr(int sdepth, int ddepth, int cn); |
There was a problem hiding this comment.
Could we add buf_size parameter instead?
There was a problem hiding this comment.
Instead of returning String? i.e. you would prefer:
CV_EXPORTS const char* convertTypeStr(int sdepth, int ddepth, int cn, char* buf, size_t buf_size);
There was a problem hiding this comment.
Yes, lets avoid calls for heap memory allocation.
|
Hmmm, I'm a bit confused as to why this PR is showing the commits from #23055 even though I have done |
Please rebase on "4.x" instead of "master" (as the target branch of this PR is "4.x"). E.g.: BTW, due to banned credentials master is not updating in our infrastructure (master is just a shadow copy of 4.x). |
…ffer size This allows removing the unsafe sprintf.
That seemed to work, thanks! |
| snprintf(buf, buf_size, "convert_%s%s_rte", typestr, (ddepth < CV_32S ? "_sat" : "")); | ||
| else | ||
| snprintf(buf, buf_max, "convert_%s_sat", typestr); | ||
| snprintf(buf, buf_size, "convert_%s_sat", typestr); |
There was a problem hiding this comment.
It would be nice to check snprintf return values.
We don't really want to go forward with truncated messages ("fail fast" is preferable).
There was a problem hiding this comment.
And do what if truncation occurs? Return NULL? None of the callers of convertTypeStr appear ready to get NULL.
Anyway, truncation is much better than a buffer overrun, which is what would happen before this PR.
There was a problem hiding this comment.
Return NULL
No, my proposal to use CV_Assert() or CV_Check*() macros (they work in both debug/release modes). These calls raise C++ exception on a problem.
Anyway, truncation is much better than a buffer overrun, which is what would happen before this PR.
Agreed. Thank you for the fix!
This first 3 commits are from #23055 (I'll rebase this after that is merged).
The last commit is an incomplete proposal to deprecated
convertTypeStrand make a new variant usingstd::string. If it's acceptable, I'll complete the PR to use the new method everywhere.