Comment 8 for bug 1417608

I reviewed libvpd, git checkout 4d57eb358f6ae45555247b3f8e8160d8f16ab9fa;
this shouldn't be considered a full security review.

Some of what I found look like serious reliability issues, perhaps even
security issues; some look like standard bugs. I've also highlighted
several examples of "assuming the cause of error", where an errno isn't
checked in event of an error, and instead the code assumes to know the
cause of an error. (This is more frustrating than dangerous.)

None of the unpacking code should be called on content supplied by
untrusted sources; callers should check owner. group, and permissions,
of whichever files supplied the data, before using the unpacking routines.

- HelperFunctions::str2chr() doesn't allocate space for NUL terminator,
  writes a NUL beyond the allocated space, this may even be exploitable.
- HelperFunctions::parsePath() 'end' variable is used uninitialzed, it
  might cause segfaults when run
- VpdRetriever::VpdRetriever() does not check system() return value for
  errors
- HelperFunctions::execCmd() hands a string directly to popen(), all
  callers should be audited
- DataItem::pack() does not validate that buf is large enough, is this safe?
- HelperFunctions::parseString() stores size_t results into 'int', I'm
  afraid large strings and malformed strings (via string::npos) may cause
  string::substr to throw exceptions
- DataItem::setValue() code to strip leading spaces probably doesn't work,
  it doesn't re-test val.at(i) after deleting the char at i, and it might
  also delete a space after a non-space, e.g. " f example" may be turned
  into "fexample", and " foo" may be turned into " foo".
- unpack_system() doesn't check operations against length of buffer
- unpack_system() doesn't check strdup() for NULL return
- unpack_dataitem() doesn't check strdup() for NULL return
- unpack_dataitem() assumes three back-to-back strings without
  verifying that it doesn't overstep bounds
- VpdDbEnv::fetch() (both versions) fail to call sqlite3_finalize() if
  ret == NULL
- fetch_component() sql injection possible via deviceID, I don't see any
  sanitization of inputs in call chains, this should use sqlite3_bind_* to
  prevent security and reliability problems
- fetch_system() sql statement could be entirely static, no need for
  sprintf()

Cases of ignoring useful error information:
- VpdDbEnv::VpdDbEnv() dbExists
- VpdRetriever::VpdRetriever() db == NULL
- HelperFunctions::file_exists(), e.g. permissions failures or rlimit
  number of open files, etc. on a file that does exist

Thanks