fix undefined behavior due to reading iterator data after modification in builder.cc

[reznikovl]

Currently, the BuildTable() function tracks the largest key as the file is iterated over and adds it to the largest field of the FileMetadata at the end. However, the Slice returned from iter->key() refers to the same memory as the iterator, which is not guaranteed to be safe after the iterator is changed (ie, when iter->next() is called). As per the documentation in iterator.h:

// Return the key for the current entry. The underlying storage for // the returned slice is valid only until the next modification of // the iterator. // REQUIRES: Valid() virtual Slice key() const = 0;

I believe this can cause undefined behavior, potentially adding junk data into meta->largest. To fix the memory bug and avoid the efficiency issue brought up in #819, I suggest seeking to the last position and then decoding that into the file, avoiding the issue.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[felipecrv]

How can the builder->Add calls modify what iter sees?

[reznikovl]

Thank you for the response @felipecrv !

How can the builder->Add calls modify what iter sees?

builder->Add won't modify it, but iter->next() might. Setting key=iter->key() shallow copies the char*, so if the underlying memory gets changed (such as when iter->next, key` will not be valid outside of the loop.