[PATCH] logproto: Fix log_proto_file_writer_flush()'s partial construction.
When log_proto_file_writer_flush() encountered a partial write, and tried to construct a self->partial buffer, that contains all the data we didn't write yet, the starting offset computation was... interesting. What we want to copy into self->partial, is whatever is left in the last, partially processed buffer (i0), then append the full contents of the rest. Calculating the starting offset of this first buffer was done in a quite convoluted way, involving the sum of the length of the buffers we already touched, the lenght of the last fully processed buffer, and the total number of bytes already written. This makes my head spin, especially since it can be simplified to substracting the number of bytes to copy from the first buffer from the total length of it. The attached patch simplifies the offset calculation, and it also fixes a subtle bug! Lets assume 5 buffers, the first one 100, the second 200, the third 300, the fourth 400 and the fifth 500 bytes, totalling to 1500 bytes. Lets assume we managed to write out the first three buffers, and half of the fourht: 800 bytes. In this case, rc will be 800, sum will be 1000, i0 will be 3, and ofs will be 200. So far, all is well. But when we try to compute the starting offset within the first partial buffer, we end up with: rc - (sum - buf[i0 - 1].length), which, when values are substituted becomes: 800 - (1000 - 300) = 100! This is obviously not correct: we've already written out half of the fourth buffer, yet, we start copying from the 100th byte instead of the 200th! After the fix, when the starting position is buffer[i0].length - ofs, we get the correct value of 200, and all is well. Reported-by: SZALAY Attila <sasa@balabit.hu> Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/logproto.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/lib/logproto.c b/lib/logproto.c index bdf9695..5047c98 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -299,7 +299,7 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + memcpy(self->partial, self->buffer[i0].iov_len - ofs, ofs); i = i0 + 1; while (i < self->buf_count) { -- 1.7.7.1
On Fri, 2011-10-28 at 23:10 +0200, Gergely Nagy wrote:
After the fix, when the starting position is buffer[i0].length - ofs, we get the correct value of 200, and all is well.
Reported-by: SZALAY Attila <sasa@balabit.hu> Signed-off-by: Gergely Nagy <algernon@balabit.hu> --- lib/logproto.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/lib/logproto.c b/lib/logproto.c index bdf9695..5047c98 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -299,7 +299,7 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + memcpy(self->partial, self->buffer[i0].iov_len - ofs, ofs); i = i0 + 1; while (i < self->buf_count) {
I think this patch has some problem too. My advise is th following line: memcpy(self->partial, self->buffer[i0].iov_base + (self->buffer[i0].iov_len - ofs), ofs); But I think that the following patch is a bit more readable: Reported-by: SZALAY Attila <sasa@balabit.hu> Signed-off-by: SZALAY Attila <sasa@balabit.hu> diff --git a/lib/logproto.c b/lib/logproto.c index 9ee6ec5..43f015f 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -271,7 +271,7 @@ static LogProtoStatus log_proto_file_writer_flush(LogProto *s) { LogProtoFileWriter *self = (LogProtoFileWriter *)s; - gint rc, i, i0, sum, ofs; + gint rc, i, i0, sum, ofs, pos; /* we might be called from log_writer_deinit() without having a buffer at all */ @@ -314,7 +314,8 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + pos = self->buffer[i0].iov_len - ofs; + memcpy(self->partial, self->buffer[i0].iov_base + pos, ofs); i = i0 + 1; while (i < self->buf_count) { -- Attila Szalay Support (L3) Team Leader e-mail: attila.szalay@balabit.com phone: +36 1 398 6707 BalaBit IT Security www.balabit.com H-1117 Budapest, Aliz street 2. This Communication is Confidential. We only send and receive email on the basis of the term set out at http://www.balabit.com/disclaimer/.
Szalay Attila <sasa@balabit.hu> writes:
diff --git a/lib/logproto.c b/lib/logproto.c index bdf9695..5047c98 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -299,7 +299,7 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + memcpy(self->partial, self->buffer[i0].iov_len - ofs, ofs); i = i0 + 1; while (i < self->buf_count) {
I think this patch has some problem too.
ACK. I really should not code half-asleep. Thanks for catching the mistake! -- |8]
On Sat, 2011-10-29 at 11:15 +0200, Gergely Nagy wrote:
Szalay Attila <sasa@balabit.hu> writes:
diff --git a/lib/logproto.c b/lib/logproto.c index bdf9695..5047c98 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -299,7 +299,7 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + memcpy(self->partial, self->buffer[i0].iov_len - ofs, ofs); i = i0 + 1; while (i < self->buf_count) {
I think this patch has some problem too.
ACK. I really should not code half-asleep. Thanks for catching the mistake!
Can you post an updated patch please? Thanks. -- Bazsi
Balazs Scheidler <bazsi@balabit.hu> writes:
On Sat, 2011-10-29 at 11:15 +0200, Gergely Nagy wrote:
Szalay Attila <sasa@balabit.hu> writes:
diff --git a/lib/logproto.c b/lib/logproto.c index bdf9695..5047c98 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -299,7 +299,7 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + memcpy(self->partial, self->buffer[i0].iov_len - ofs, ofs); i = i0 + 1; while (i < self->buf_count) {
I think this patch has some problem too.
ACK. I really should not code half-asleep. Thanks for catching the mistake!
Can you post an updated patch please?
Sasa already posted one, but included below is his full version of the patch:
From 012a245281da19970d4e0b863e833e1909c2eea6 Mon Sep 17 00:00:00 2001 From: Szalay Attila <sasa@balabit.hu> Date: Sat, 29 Oct 2011 11:29:18 +0200 Subject: [PATCH] logproto: In case of partial writing there were a problem in the pos calculating (fixes: #24459)
If the log messages has different length and only a partial write happen the log_proto_file_writer_flush there are a possible buffer under/overflow happen. The problematic part is in the calculation about the last written byte of the last written message. The calculation is not just too difficult to follow but use the wrong message length in it. Because of this ther are buffer under/overflow may happen or starting to read the message in wrong position, causing messing the log. --- lib/logproto.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/logproto.c b/lib/logproto.c index 9ee6ec5..43f015f 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -271,7 +271,7 @@ static LogProtoStatus log_proto_file_writer_flush(LogProto *s) { LogProtoFileWriter *self = (LogProtoFileWriter *)s; - gint rc, i, i0, sum, ofs; + gint rc, i, i0, sum, ofs, pos; /* we might be called from log_writer_deinit() without having a buffer at all */ @@ -314,7 +314,8 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + pos = self->buffer[i0].iov_len - ofs; + memcpy(self->partial, self->buffer[i0].iov_base + pos, ofs); i = i0 + 1; while (i < self->buf_count) { -- 1.7.7.1 -- |8]
On Sat, 2011-10-29 at 14:13 +0200, Gergely Nagy wrote:
Balazs Scheidler <bazsi@balabit.hu> writes:
On Sat, 2011-10-29 at 11:15 +0200, Gergely Nagy wrote:
Szalay Attila <sasa@balabit.hu> writes:
diff --git a/lib/logproto.c b/lib/logproto.c index bdf9695..5047c98 100644 --- a/lib/logproto.c +++ b/lib/logproto.c @@ -299,7 +299,7 @@ log_proto_file_writer_flush(LogProto *s) /* allocate and copy the remaning data */ self->partial = (guchar *)g_malloc(self->partial_len); ofs = sum - rc; /* the length of the remaning (not processed) chunk in the first message */ - memcpy(self->partial, self->buffer[i0].iov_base + rc - (i0 > 0 ? (sum - self->buffer[i0 - 1].iov_len) : 0), ofs); + memcpy(self->partial, self->buffer[i0].iov_len - ofs, ofs); i = i0 + 1; while (i < self->buf_count) {
I think this patch has some problem too.
ACK. I really should not code half-asleep. Thanks for catching the mistake!
Can you post an updated patch please?
Applied to 3.3, thanks. -- Bazsi
participants (3)
-
Balazs Scheidler
-
Gergely Nagy
-
Szalay Attila