I still think this change is OK, as AFAIK sprintf terminates
the final string (not the "%s" itself because everything after
it wouldn't be printed, e.g. "%s %s" wouldn't work), from the man:
Upon successful return, these functions return the number of
characters
printed (not including the trailing '\0' used to end output to
strings).
Simple test case:
#include <stdio.h>
char buf[] = "abcdefgh";
int main(int argc, char *argv[])
{
sprintf(buf, "%s", "123");
printf(buf);
return 0;
}
$ ./test
123
thanks & regards
Jaroslav
----- Original Message -----
The ReadFile change makes sense but we need to retain the line:
data[len + bytes] = 0;
From the sprintf man page:
"Characters from the array are written up to (but not includ‐
ing) a terminating null byte ('\0');"
On Sat, Jun 4, 2011 at 9:03 PM, Jaroslav Škarvada
<jskarvad(a)redhat.com> wrote:
> Add null termination to the buf in the read_output in windows
> services code.
> The missing null termination could cause random crashes (DoS).
> ---
> src/lib/services_windows.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/lib/services_windows.c b/src/lib/services_windows.c
> index 80b9a39..85d33d7 100644
> --- a/src/lib/services_windows.c
> +++ b/src/lib/services_windows.c
> @@ -87,16 +87,16 @@ read_output(HANDLE h, char **output, int
> *length)
> char *data = NULL;
>
> while (1) {
> - if (ReadFile(h, buf, BUFSIZE, &bytes, NULL) == 0) {
> + if (ReadFile(h, buf, BUFSIZE - 1, &bytes, NULL) == 0) {
> break;
> } else if (bytes == 0) {
> break;
> }
>
> + buf[bytes] = 0;
> data = realloc(data, len + (int) bytes + 1);
> mh_info("Read %d: %.*s", len, (int) bytes, buf);
> sprintf(data + len, "%s", buf);
> - data[len + bytes] = 0;
> len += (int)bytes;
> }
>
> --
> 1.7.5.2
>
> _______________________________________________
> Matahari mailing list
> Matahari(a)lists.fedorahosted.org
>
https://fedorahosted.org/mailman/listinfo/matahari
>