[mrtdump branch] Segfault when using a long filename for mrtdump

[Baptiste Jonglez]

Thanks for the quick patch Pavel, it seems to work fine!

On Wed, Apr 27, 2016 at 10:27:40AM +0200, Pavel Tvrdík wrote:
> Hi Baptiste,
> 
> the bug should be fixed! Pull a new commit from GIT or download a mail
> attachment.
> 
> Thanks,
> Pavel
> 
> On 2016-04-26 17:37, Pavel Tvrdík wrote:
> >Hi Baptiste!
> >
> >On 2016-04-26 16:46, Baptiste Jonglez wrote:
> >>Hi,
> >>
> >>I have been experimenting with the mrtdump branch (commit cc4eee62) on
> >>Debian jessie.
> >
> >Thank you for testing experimental code from GIT!
> >
> >>
> >>However, when using a moderately long filename, Bird crashes.
> >>For instance:
> >>
> >>    birdc6 'mrtdump routes to "/bird/mrtdump/rib.ipv6.20160426.1209"'
> >>    BIRD 1.5.0 ready.
> >>    Connection closed by server.
> >>
> >>The logs are the following:
> >>
> >>    bird6[8314]: Unable to open file "<too-long>" for MRT dump of table
> >>master
> >>    systemd[1]: bird6.service: main process exited, code=killed,
> >>status=11/SEGV
> >>    systemd[1]: Unit bird6.service entered failed state.
> >>    kernel: bird6[8314]: segfault at 18 ip 00007fb05162a905 sp
> >>00007ffe1194f5a0 error 4 in bird6[7fb051602000+76000]
> >>
> >>It looks like TM_DATETIME_BUFFER_SIZE, as used in tm_format_datetime(),
> >>is
> >>really too small (32 bytes).  Also, there seems to be an issue with
> >>error
> >>handling (segfault when tm_format_datetime returns unexpected data).
> >>
> >>When defining a higher value for TM_DATETIME_BUFFER_SIZE, bird does not
> >>crash anymore, but there is certainly a better solution.
> >
> >Yes, exactly. I'll send a patch tomorrow!
> >
> >>Thanks,
> >>Baptiste

> From f7cd90113c281022acc50c42bcce1b44b7103395 Mon Sep 17 00:00:00 2001
> From: Pavel Tvrdik <pawel.tvrdik at gmail.com>
> Date: Wed, 27 Apr 2016 10:12:26 +0200
> Subject: [PATCH] MRT Dump: Fix bug with longer filename formats
> 
> Length of filename format is based on PATH_MAX. Better treating with
> filename format buffer size overflow, no segmentation fault.
> ---
>  nest/route.h    |  2 +-
>  nest/rt-table.c | 31 ++++++++++++++++++++-----------
>  2 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/nest/route.h b/nest/route.h
> index 0d3a85f..9ea9563 100644
> --- a/nest/route.h
> +++ b/nest/route.h
> @@ -628,6 +628,6 @@ struct mrt_table_config *mrt_table_new_config(void);
>  struct mrt_table_dump_ctx;
>  int is_route_good_for_table_dump(struct mrt_table_dump_ctx *state, rte *e);
>  //char *mrt_table_dump_config_get_filename_fmt(struct rtable *rtable);
> -void mrt_table_dump_init_file_descriptor(struct mrt_table_dump_ctx *state);
> +struct rfile *mrt_table_dump_init_file_descriptor(struct mrt_table_dump_ctx *state);
>  
>  #endif
> diff --git a/nest/rt-table.c b/nest/rt-table.c
> index 4420613..9550dd3 100644
> --- a/nest/rt-table.c
> +++ b/nest/rt-table.c
> @@ -2752,10 +2752,13 @@ mrt_table_dump_cmd_step(void *mrt_table_dump_ctx)
>      rfree(state->step);
>      if (state->config.c.config)
>        config_del_obstacle(state->config.c.config);
> -    log(L_INFO "MRT dump of table %s was saved into file \"%s\"", state->rtable->name, state->file_path);
> -    if (state->config.c.cli)
> +    if (state->rfile)
>      {
> -      cli_printf(state->config.c.cli, 13, "Dump of table %s was saved into file \"%s\"", state->rtable->name, state->file_path);
> +      log(L_INFO "MRT dump of table %s was saved into file \"%s\"", state->rtable->name, state->file_path);
> +      if (state->config.c.cli)
> +      {
> +        cli_printf(state->config.c.cli, 13, "Dump of table %s was saved into file \"%s\"", state->rtable->name, state->file_path);
> +      }
>      }
>      rt_unlock_table(state->rtable);
>      mb_free(state->file_path);
> @@ -2783,10 +2786,12 @@ mrt_table_dump_init(struct mrt_table_dump_ctx *state)
>  
>    state->state = MRT_STATE_RUNNING;
>    state->rib_sequence_number = 0;
> -  mrt_table_dump_init_file_descriptor(state);
>    mrt_rib_table_alloc(&state->rib_table);
>  
> -  bgp_mrt_peer_index_table_dump(state);
> +  if (mrt_table_dump_init_file_descriptor(state))
> +    bgp_mrt_peer_index_table_dump(state);
> +  else
> +    state->state = MRT_STATE_COMPLETED;
>  }
>  
>  struct mrt_table_dump_ctx *
> @@ -2887,7 +2892,7 @@ mrt_table_dump_get_realpath(const char *filename)
>    return path;
>  }
>  
> -void
> +struct rfile *
>  mrt_table_dump_init_file_descriptor(struct mrt_table_dump_ctx *state)
>  {
>    char *tablename = state->config.table_cf->name;
> @@ -2895,12 +2900,15 @@ mrt_table_dump_init_file_descriptor(struct mrt_table_dump_ctx *state)
>  
>    if (filename_fmt)
>    {
> -    struct timeformat timestamp_fmt = {
> -	.fmt1 = filename_fmt,
> -    };
> +    char filename[BIRD_PATH_MAX];
> +    struct tm *tm = localtime(&now_real);
> +    if (!strftime(filename, sizeof(filename), filename_fmt, tm))
> +    {
> +      log(L_ERR "Invalid filename format \"%s\"", filename_fmt);
> +      mb_free(filename_fmt);
> +      return NULL;
> +    }
>  
> -    char filename[TM_DATETIME_BUFFER_SIZE];
> -    tm_format_datetime(filename, &timestamp_fmt, now);
>      state->rfile = tracked_fopen(rt_table_pool, filename, "a");
>  
>      const char *filename_fullpath = mrt_table_dump_get_realpath(filename);
> @@ -2933,6 +2941,7 @@ mrt_table_dump_init_file_descriptor(struct mrt_table_dump_ctx *state)
>        cli_msg(13, "Parsing filename filename_fmt \"%s\" for table %s failed", mrt_table_dump_config_get_filename_fmt(state), tablename);
>      }
>    }
> +  return state->rfile;
>  }
>  
>  static void

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20160427/a298b574/attachment.asc>