Ok, found some more flaws. Most are minor and easy to fix. But the
bsearch has some major issues that I couldn't ignore, sorry ;)

View Change

16 comments:

• Commit Message:

  • Patch Set #22, Line 14: to

    This `to` seems spurious.

  • Patch Set #22, Line 22: mostly copied from

    Rather `based on` now?

• File cli_classic.c:

  • Patch Set #22, Line 642: && !fmapfile

    I stared at this for a moment, now I'm sure the `!fmapfile` is
    redundant.

• File flashrom.8.tmpl:

  • Patch Set #22, Line 52: [\fB\-i\fR <image>]] [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR]]

    This line is unnecessarily long now. Please break before the
    `-n`. Would still look good and fit an 80 chars view if inden-
    tation is reduced by 3 spaces (the current 15 spaces indent is
    odd anyway).

  • Patch Set #22, Line 202: partioning

    par*ti*tioning

  • Patch Set #22, Line 217: partioning

    partitioning

• File fmap.h:

  • Patch Set #22, Line 3: -present

    I know it has been discussed already. But this is incredibly
    annoying. While everybody seems to know that this `-present`
    is not legally binding and nonsense, coming from a huge com-
    pany like Facebook (that should know better) feels quite of-
    fensive. Do they really want to be the Disney of open source?

    I won't block it. Just saying... you can't complain about
    this to your superiors often enough. Just every day "Every
    minute a(nother) copy of this is online, it's lowering Face-
    book's reputation".

  • Patch Set #22, Line 45: #define FMAP_VER_MINOR 1 /* this header's FMAP minor version */

    Didn't check earlier versions, but now this is unused? shouldn't
    we check the minor version too?

  • Patch Set #22, Line 67: int is_valid_fmap(const struct fmap *fmap);

    nit, could be static now

• File fmap.c:

  • Patch Set #22, Line 1:

    nit, missing initial line break

  • Patch Set #22, Line 203: return 1;

    leaking `fmap`

  • Patch Set #22, Line 260:

    			fmap_len = fmap_size(fmap);
    			struct fmap *tmp = fmap;
    			fmap = realloc(fmap, fmap_len);
    			if (!fmap) {
    				msg_gerr("Failed to realloc.\n");
    				free(tmp);
    				goto _free_ret;
    			}
    
    			if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap),
    						offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) {
    				msg_cerr("Cannot read %zu bytes at offset %06zx\n",
    						fmap_len - sizeof(*fmap), offset + sizeof(*fmap));
    				/* Treat read failure to be fatal since this
    				 * should be a valid, usable fmap. */
    				goto _free_ret;
    			}
    
    			*fmap_out = malloc(fmap_len);
    			if (*fmap_out == NULL) {
    				msg_gerr("Out of memory.\n");
    				goto _free_ret;
    			}
    
    			memcpy(*fmap_out, fmap, fmap_len);
    

    This path is flawed a little. At least it's missing an unconditional
    `break` after the memcpy(). But this also shows that everything after
    setting `fmap_found = 1` could happen outside the loops. So optio-
    nally we could refactor as follows:

      for (stride = ... ) {
              ...
              for (offset = ... ) {
                      ...
                      if (is_valid_fmap(fmap)) {
                              msg_gdbg(...
                              fmap_found = 1;
                              break;
                      }

                      msg_gerr(...
                      ret = 2;
              }
              if (fmap_found)
                      break;
      }

      if (!fmap_found)
              goto _free_ret;

      /* code from above follows, moved out of the loops */

    Um, with that said, there seems to be no valid path to hit the
    realloc() a second time. Every failure after the realloc() is
    fatal, and if we found and read a valid fmap, we should stop sear-
    ching, right?

  • Patch Set #22, Line 292: if (fmap_found)

    This would still be set after a read failure, for instance. So it's
    probably safest to place the `ret = 0` right after the memcpy() to
    `*fmap_out`.

  • Patch Set #22, Line 321: a fmap

    How do you pronounce it? Is it `*a* fmap` or `*an* eff-map`?

• File libflashrom.c:

  • Patch Set #22, Line 433: {

    Ugly, but as we were too lazy for proper deserialization again,
    we should guard here with __FLASHROM_LITTLE_ENDIAN__ like it's
    done in flasrom_layout_read_from_ifd().

  • Patch Set #22, Line 468: {

    Same guard here.

To view, visit change 23203. To unsubscribe, or for help writing mail filters, visit settings.