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

[Pavel Tvrdík]

Glad to hear it!

On 2016-04-27 10:55, Baptiste Jonglez wrote:
> 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