On Oct 3, 2013, at 10:00 AM, Michael Jennings <m...@lbl.gov> wrote:
> On Thu, Oct 3, 2013 at 9:44 AM, David Bigagli <da...@schedmd.com> wrote: > >> I don't know the details of the segfault but the code in question is >> correct. If you decrease the length then the file cmdlen: >> >> cmdlen = PMII_MAX_COMMAND_LEN - remaining_len; >> >> will not be correct and wrong length will be sent to the pmi2 server. >> >> This code is taken verbatim from mpich2-1.5, precisely from: >> mpich2-1.5/src/pmi/pmi2/simple2pmi.c > > If that's true, the original code is also wrong. :-) Indeed - bottom line is that just because it is in mpich doesn't mean it is correct :-) > > Ralph is correct here. Let's walk through this: > > char cmdbuf[PMII_MAX_COMMAND_LEN]; > char *c = cmdbuf; > int remaining_len = PMII_MAX_COMMAND_LEN; > > /* leave space for length field */ > memset(c, ' ', PMII_COMMANDLEN_SIZE); > c += PMII_COMMANDLEN_SIZE; > > Now let's say PMII_MAX_COMMAND_LEN is 4 and PMII_COMMANDLEN_SIZE is 1. > cmdbuf and c start out pointing to a 4-byte buffer. We then set the > first char to ' ' and point c to the 2nd position in the buffer. > remaining_len is still 4. > > Now, later we have: > > ret = snprintf(c, remaining_len, "thrid=%p;", resp); > > This will segfault because it thinks it can write 4 chars to c when, > in fact, it can only write 3. Obviously these small buffer sizes are > contrived, but they illustrate the point. :-) > > If the resulting command length value depends on this bug being > present, some mitigation will have to happen later on - either changes > in every snprintf() call, or a last-minute alteration to the command > length (probably the wiser idea). Thanks - that matches our analysis too > > Michael > > -- > Michael Jennings <m...@lbl.gov> > Senior HPC Systems Engineer > High-Performance Computing Services > Lawrence Berkeley National Laboratory > Bldg 50B-3209E W: 510-495-2687 > MS 050B-3209 F: 510-486-8615
