The first memcpy() call in puts_clbk_str_w() confuses character count and byte count. It uses the number of characters (out->len) as number of bytes. This leaves half of the buffer undefined.
Interestingly, the second memcpy() call in the same function is correct.
This bug potentially makes applications expose internal (secret) data. Usually, the destination buffer is on the stack, and the stack often contains secrets. Therefore, one could argue that this bug constitutes a security vulnerability. --- dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len;
if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len; out->len = 0; return -1;
In general, I think you want to send this to wine-patches, not here.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann max@duempel.org wrote:
The first memcpy() call in puts_clbk_str_w() confuses character count and byte count. It uses the number of characters (out->len) as number of bytes. This leaves half of the buffer undefined.
Interestingly, the second memcpy() call in the same function is correct.
This bug potentially makes applications expose internal (secret) data. Usually, the destination buffer is on the stack, and the stack often contains secrets. Therefore, one could argue that this bug constitutes a security vulnerability.
It'd be hard to make that argument convincingly. That's neither here nor there, I suppose, but...
---
dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len;
if(out->len < len) {
memcpy(out->buf, str, out->len);
memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is. --Juan
On 2013/05/07 17:46, Juan Lang juan.lang@gmail.com wrote:
In general, I think you want to send this to wine-patches, not here.
True, I resent it to wine-patches yesterday already, when I noticed that.
if(out->len < len) {
memcpy(out->buf, str, out->len);
memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is.
Sure?
out->buf is an "APICHAR*" (see printf.h), and adding out->len advances the pointer by "out->len * sizeof(APICHAR)" bytes.
Am I missing something?
Max
On 05/07/13 17:46, Juan Lang wrote:
In general, I think you want to send this to wine-patches, not here.
This patch was also sent to wine-patches.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann <max@duempel.org mailto:max@duempel.org> wrote:
--- dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len; if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is.
out->buf is of APICHAR* type, so it's updated correctly. The patch looks good for me.
On Tue, May 7, 2013 at 9:10 AM, Piotr Caban piotr.caban@gmail.com wrote:
On 05/07/13 17:46, Juan Lang wrote:
In general, I think you want to send this to wine-patches, not here.
This patch was also sent to wine-patches.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann <max@duempel.org
mailto:max@duempel.org> wrote:
--- dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len; if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is.
out->buf is of APICHAR* type, so it's updated correctly. The patch looks good for me.
Thanks, Piotr. Max, my apologies for the sloppy review. --Juan
-
Juan Lang -
Max Kellermann -
Piotr Caban