ext/mysqlnd: Refactor usage of strlcpy() #17185
Conversation
The two calls that MySQLnd does to this handler all pass a buffer the same size as the error_msg field Thus we know that we can just memcpy the error message into the buffer.
Girgias
requested review from
bukka,
kamil-tekiela and
SakiTakamachi
as code owners
ext/mysqlnd/mysqlnd_loaddata.c
Outdated
|
|
||
| DBG_ENTER("mysqlnd_local_infile_read"); | ||
|
|
||
| count = (int) php_stream_read(info->fd, (char *) buf, buf_len); | ||
| ssize_t count = php_stream_read(info->fd, (char *) buf, buf_len); |
There was a problem hiding this comment.
The return type of int mysqlnd_local_infile_read() should be changed as well, otherwise the count < 0 check bellow may be false, but the return value may still be < 0 due to conversion to int.
There was a problem hiding this comment.
Where does the conversion to int take place?
There was a problem hiding this comment.
It happens when returning count, as the return type is int
There was a problem hiding this comment.
It's an unrelated change so it should be probably removed from this PR and different PR created for this.
|
Why is this actually being done? I understand that strlcpy might have slightly bigger overhead and maybe not supported on all platfoms but I don't exactly understand why it should be done here (error handling part)? It seems kind of like waste of time to even review this or am I missing anything important? |
|
Because |
|
I think it would be fine to merge this but maybe without the |
|
I'm just on holiday for the Xmas break, I'll get round to updating the PR after it (possibly on the 27th) |
|
Well I don't see how it's worse than strcpy and it's available in almost any libc (even glibc added it...) AFAIC. I don't have a big issue with removal though. Just thought that there might be more important things to do. |
The two calls that MySQLnd does to this handler all pass a buffer the same size as the error_msg field Thus we know that we can just memcpy the error message into the buffer.