[syslog-ng] [PATCH] [logproto.c] "Invalid Frame Header" in syslog protocol

[Balazs Scheidler]

On Tue, 2012-01-24 at 18:10 +0100, Gergely Nagy wrote:
> When using the syslog protocol, in case when we received an EAGAIN
> during an attempt to send the framing, the frame was not resent,
> resulting in protocol errors and invalid frame header messages on the
> other end.
> 
> This patch corrects the problem by not using
> log_proto_text_client_post to send framed messages, but a wrapper that
> can resume sending the frame too.
> 
> Signed-off-by: Fried Zoltan <deirf at balabit.hu>
> Signed-off-by: Gergely Nagy <algernon at balabit.hu>
> ---
>  lib/logproto.c |   64 ++++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 42 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/logproto.c b/lib/logproto.c
> index 16ca0d7..e2cf944 100644
> --- a/lib/logproto.c
> +++ b/lib/logproto.c
> @@ -35,6 +35,26 @@
>  #include <sys/uio.h>
>  #include <limits.h>
>  
> +
> +typedef struct _LogProtoTextClient
> +{
> +  LogProto super;
> +  guchar *partial;
> +  gsize partial_len, partial_pos;
> +} LogProtoTextClient;
> +
> +#define LPFCS_FRAME_INIT    0
> +#define LPFCS_FRAME_SEND    1
> +#define LPFCS_MESSAGE_SEND  2
> +
> +typedef struct _LogProtoFramedClient
> +{
> +  LogProtoTextClient super;
> +  gint state;
> +  gchar frame_hdr_buf[9];
> +  gint frame_hdr_len, frame_hdr_pos;
> +} LogProtoFramedClient;
> +
>  gboolean
>  log_proto_set_encoding(LogProto *self, const gchar *encoding)
>  {
> @@ -70,14 +90,6 @@ log_proto_free(LogProto *s)
>    g_free(s);
>  }
>  
> -
> -typedef struct _LogProtoTextClient
> -{
> -  LogProto super;
> -  guchar *partial;
> -  gsize partial_len, partial_pos;
> -} LogProtoTextClient;
> -
>  static gboolean
>  log_proto_text_client_prepare(LogProto *s, gint *fd, GIOCondition *cond)
>  {
> @@ -144,7 +156,7 @@ log_proto_text_client_flush(LogProto *s)
>   * successfully sent this message, or if it should be resent by the caller.
>   **/
>  static LogProtoStatus
> -log_proto_text_client_post(LogProto *s, guchar *msg, gsize msg_len, gboolean *consumed)
> +log_proto_client_post_writer(LogProto *s, guchar *msg, gsize msg_len, gboolean *consumed, gboolean syslog_proto)
>  {
>    LogProtoTextClient *self = (LogProtoTextClient *) s;
>    gint rc;
> @@ -173,6 +185,14 @@ log_proto_text_client_post(LogProto *s, guchar *msg, gsize msg_len, gboolean *co
>         */
>        return rc;
>      }
> +  else if (self->partial_len != 0 && !self->partial && syslog_proto)
> +  {
> +    LogProtoFramedClient *framed_self = (LogProtoFramedClient *) s;
> +    self->partial_pos =0;
> +    self->partial_len =0;
> +    framed_self->state = LPFCS_FRAME_INIT;
> +    return rc;
> +  }

sorry, but I won't commit this. you are casting "self" to a subclass in
a more generic class. I don't really see why this isn't handled by the
"if" in log_proto_framed_client_post() right after the call to
log_proto_text_client_post().

Here's the code that should do the same:

    case LPFCS_MESSAGE_SEND:
      rc = log_proto_text_client_post(s, msg, msg_len, consumed);
 
      /* NOTE: we don't check *consumed here, as we might have a pending
       * message in self->partial before we begin, in which case *consumed
       * will be FALSE. */
      
      if (rc == LPS_SUCCESS && self->super.partial == NULL)
        {
          self->state = LPFCS_FRAME_INIT;
        }
      return rc;

If a partial message is set and we get an EAGAIN, then log_proto_text_client_flush() 
returns LPS_SUCCESS and self->partial != NULL, which in turn leaves the state 
LPFCS_MESSAGE_SEND, causing the partial message to be resent.

What am I missing? can you describe the concrete scenario that triggers the bug?


Thanks.

-- 
Bazsi