ctdb: stack leakage over wire

[Rusty Russell]

Hi all,

   Running valgrind showed that we're leaking uninitialized vars over the
wire.  In particular on 64 bit x86:

        struct takeover_run_reply {
                uint32_t pnn;
                uint64_t srvid;
        };

The padding bytes between the two are uninitialized:

        static void ctdb_health_callback(struct ctdb_context *ctdb, int status, void *p)
        {
        ...
                struct takeover_run_reply rd;
        ...
                rd.pnn   = ctdb->pnn;
                rd.srvid = CTDB_SRVID_TAKEOVER_RUN_RESPONSE;

This then gets memcpy'd by generic routines into the packet.  Fixing this case
is easy, but there are others.  There are several solutions I can see:

1) Try to remember to explicitly set stuff everywhere (whack-a-mole),
2) Explicit marshal/unmarshall funcs for each type (slightly less fragile, as
   only those funcs need to remember to zero),
3) Set attribute((packed)) (breaks wire format, assumes gcc extension),
4) Bite the bullet and marshall/unmarshall into a portable format (breaks
   wire format, but allows potential for cross-platform ctdb).

Thoughts welcome...
Rusty.

[Stefan (metze) Metzmacher]

Rusty Russell schrieb:

> Hi all,
>
>    Running valgrind showed that we're leaking uninitialized vars over the
> wire.  In particular on 64 bit x86:
>
> struct takeover_run_reply {
> uint32_t pnn;
> uint64_t srvid;
> };
>
> The padding bytes between the two are uninitialized:
>
> static void ctdb_health_callback(struct ctdb_context *ctdb, int status, void *p)
> {
> ...
> struct takeover_run_reply rd;
> ...
> rd.pnn   = ctdb->pnn;
> rd.srvid = CTDB_SRVID_TAKEOVER_RUN_RESPONSE;
>
> This then gets memcpy'd by generic routines into the packet.  Fixing this case
> is easy, but there are others.  There are several solutions I can see:
>
> 1) Try to remember to explicitly set stuff everywhere (whack-a-mole),
> 2) Explicit marshal/unmarshall funcs for each type (slightly less fragile, as
>    only those funcs need to remember to zero),
> 3) Set attribute((packed)) (breaks wire format, assumes gcc extension),
> 4) Bite the bullet and marshall/unmarshall into a portable format (breaks
>    wire format, but allows potential for cross-platform ctdb).

I would prefer 4), but it has a lot of impact on the current code layout
and maybe also impact performance...

metze

[ronnie sahlberg]

For short term I think 1 is sufficient.

Longer term, 4 would be very desireable.


On Thu, Nov 5, 2009 at 5:04 PM, Rusty Russell <rusty@...> wrote:

> Hi all,
>
>   Running valgrind showed that we're leaking uninitialized vars over the
> wire.  In particular on 64 bit x86:
>
>        struct takeover_run_reply {
>                uint32_t pnn;
>                uint64_t srvid;
>        };
>
> The padding bytes between the two are uninitialized:
>
>        static void ctdb_health_callback(struct ctdb_context *ctdb, int status, void *p)
>        {
>        ...
>                struct takeover_run_reply rd;
>        ...
>                rd.pnn   = ctdb->pnn;
>                rd.srvid = CTDB_SRVID_TAKEOVER_RUN_RESPONSE;
>
> This then gets memcpy'd by generic routines into the packet.  Fixing this case
> is easy, but there are others.  There are several solutions I can see:
>
> 1) Try to remember to explicitly set stuff everywhere (whack-a-mole),
> 2) Explicit marshal/unmarshall funcs for each type (slightly less fragile, as
>   only those funcs need to remember to zero),
> 3) Set attribute((packed)) (breaks wire format, assumes gcc extension),
> 4) Bite the bullet and marshall/unmarshall into a portable format (breaks
>   wire format, but allows potential for cross-platform ctdb).
>
> Thoughts welcome...
> Rusty.
>