Add test showing error in StreamingMemoryObject.setKnownObjectSize().

[kschimpf]

Summary

The existing code for method StreamingMemoryObject.fetchToPos does not
respect the corresonding call to setKnownObjectSize(). As a result, it
allows the StreamingMemoryObject to read bytes past the object size.

[kschimpf]

Filed bug https://llvm.org/bugs/show_bug.cgi?id=23174 to track fixing this bug.

[kschimpf]

Fix StreamingMemoryObject to respect call to setKnownObjectSize().

[• rafael]

sorry, looks like fab didn't sent the comment before. Trying agin.

include/llvm/Support/StreamingMemoryObject.h

84 ↗

(On Diff #23517)

I still don't get it.

The comment says

Returns true if Pos can be read.

Now , assume you start reading a bitcode file over http. You get the header of the wrapper saying the size and ObjectSize is set.
After that your network connection goes down and GetBytes will fail to read more data.

fetchToPos will incorrectly say that Pos can be read, no?

[kschimpf]

Fix issues raised in diff 23517.

[kschimpf]

include/llvm/Support/StreamingMemoryObject.h

84 ↗

(On Diff #23517)

Good point. I didn't deal with that case. I only dealt with the case where BytesRead was larger than ObjectSize.

Fixing so that both cases are handled.

[jvoung]

Mostly thinking out loud, but a few suggestions in the sea of text too.

include/llvm/Support/StreamingMemoryObject.h

78 ↗

(On Diff #23683)

Just thinking out loud here to think through what's going on... it is a bit confusing that this has to deal with an ObjectSize, and the fact that it can be different from BytesRead is just more fun =)

If ObjectSize is known, and BytesRead goes above ObjectSize, doesn't that mean that it already read beyond the ObjectSize from the perspective of the Streamer? The amount to read is just kChunkSize. This will adjust the "position" of the Streamer for its next call to GetBytes... if there is a next call. The Streamer doesn't let you seek backward, so you can no longer make the next GetBytes start at exactly the ObjectSize boundary.

So, why is this okay? My impression is that this is okay because the Streamer is owned by this class, and so it should not be shared, so adjusting the position of Streamer beyond the ObjectSize doesn't matter too much.

Okay, so what does matter?

The bounds determined by BytesRead does matter, since that affects what readBytes() will do. readBytes() does not check ObjectSize... and that's what this extra check fixes. On the other hand readBytes() calls fetchToPos, so it is in a way checking ObjectSize (just tucked away here).

81 ↗

(On Diff #23683)

Following up on Rafael's example, why not set ObjectSize = BytesRead here unconditionally? At this point, if ObjectSize != 0, then BytesRead <= ObjectSize.

Otherwise, if you call fetchToPos(Pos) afterwards with BytesRead < Pos < ObjectSize, then EOOReached is true and it will say that Pos is readable (since the check on line 71 is Pos < ObjectSize, instead of Pos < BytesRead).

Maybe that earlier condition on line 71 should just be Pos < BytesRead to be consistent with readBytes using "BytesRead" as the boundary at EOF, instead of ObjectSize as the boundary at EOF. It will also be consistent with the "return Pos < BytesRead;" at the end of this function on line 87.

On the other hand, getExtent() and isValidAddress() use ObjectSize, and it will be inconsistent if the stream is ever truncated to be less than the known ObjectSize, but the callers will just have to rely on what readBytes ultimately says.

unittests/Support/StreamingMemoryObject.cpp

22 ↗

(On Diff #23683)

How about having a second unittest class which sets a limit on how many GetBytes can be pulled. Something like a SizedNullDataStreamer class (or whatever you want to call it).

Then you can have a tests for:

(a) the size isn't known, and the end of object is solely based on what GetBytes() returns vs the requested len.

(b) the one case Rafael brought up -- when the size is "known", but something unexpected happens like the http connection dies before hitting the known size. It could also be that the wrapper lies about the size (maybe someone is trying to trigger a bug).

[kschimpf]

Clean up concepts of fetching bytes from ObjectSize.

[kschimpf]

Rewrote fetchToPos to only worry about reading, and readBytes to check if input is truncated because ObjectSize is set. This simplifies fetchToPos considerably, and only marginally complicates readBytes.

include/llvm/Support/StreamingMemoryObject.h

78 ↗

(On Diff #23683)

I agree that the concepts of ObjectSize, NextBytes, and EndOfStream() are confused by the way this code is written. Separating these concepts out (see comment below), so that things are (hopefully) cleaner.

81 ↗

(On Diff #23683)

First off, when ObjectSize != 0, BytesRead <= ObjectSize is only guaranteed to be true because setKnownObjectSize would have changed it if BytesRead > ObjectSize. In general, since we don't control when setKnownObjectSize is called, there is no real association between BytesRead and ObjectSize.

The fundamental problem is that we have two notions of EOF (BytesRead and ObjectSize), and their value is independent of each other.

This CL tries to reduce overhead by making them the same if setKnownObjectSize is called.

Alternatively, we can keep these notions separate. Hence, fetchToPos will always do a read and set NextBytes to the end of the read. However, then we need to modify readBytes to check that the requested bytes doesn't exceed ObjectSize.

Updating the CL to match this concept.

[jvoung]

Thanks -- looks okay to me, but wait for Rafael to take a look too since he some earlier questions?

include/llvm/Support/StreamingMemoryObject.h

66 ↗

(On Diff #23741)

Can you also add a period at the end of the sentence while you are updating this comment?

lib/Support/StreamingMemoryObject.cpp

91 ↗

(On Diff #23741)

Maybe add a brief comment about why ObjectSize can be < BytesRead.

111 ↗

(On Diff #23741)

Why not keep the reserve?

[kschimpf]

Rafael,

Jan has reviewed this CL but wants your final say on accepting (since you had questions earlier). Could you please take a look. Thanks

[• rafael]

It would be better for both APIs to see the end of data in the same way.

The code organization actually makes that easy now, no?:

Just replace the

return true;

on line 80 with

return !ObjectSize || Pos < ObjectSize;

Considering all the cases:

• Asked to read until a valid position and we manage to read it: We will return true since we either found the object size or it has to be larger than pos (since pos is valid)
• Asked to read too much and we actually manage to do it. ObjectSize must be set for it to be legal to put data after a bitcode file, so we will detect this.
• Asked to read too much, and the GetBytes fails, the check for eof will return false.

include/llvm/Support/StreamingMemoryObject.h

75 ↗

(On Diff #23741)

Why only == 0?

The previous logic looks correct If we ask for 100 bytes but only get 50, we are at the end of the stream.

[kschimpf]

• Merge branch 'master' of http://llvm.org/git/llvm into strmfix
• Merge branch 'master' of http://llvm.org/git/llvm into strmfix
• Fix issues with diff 23741.

[kschimpf]

• Fix order of methods in StreamingMemoryObject.cpp

[kschimpf]

Please take a look at Diff 25053. Thanks.

include/llvm/Support/StreamingMemoryObject.h

66 ↗

(On Diff #23741)

Done.

75 ↗

(On Diff #23741)

I used bytes == 0 following how read works. For the case it is hooked up to a pipe, it may return only some of the requested bytes (not necessarily all). This is done when the pipe doesn't have all the bytes requested. In that case, it returns what it does have (and at least one character). It only returns 0 if you reach the pipe has been closed, and you are at the end of the pipe contents.

This matches what we might expect from a data streamer​. Note that many people misuse read and assume that it will always return the number of requested bytes, unless eof has been reached. It is simply not (always) true. I have been burned too many times and that is why I used bytes == 0.

81 ↗

(On Diff #23741)

Fixed return value, as suggested by Rafael.

lib/Support/StreamingMemoryObject.cpp

91 ↗

(On Diff #23741)

Done.

111 ↗

(On Diff #23741)

Added it back. Also set EOFReached if ObjectSize <= BytesRead.

[kschimpf]

ping!

[• rafael]

LGTM

llvm/trunk/include/llvm/Support/StreamingMemoryObject.h

private:
   const static uint32_t kChunkSize = 4096 * 4;
   mutable std::vector<unsigned char> Bytes;
   std::unique_ptr<DataStreamer> Streamer;
   mutable size_t BytesRead;   // Bytes read from stream
   size_t BytesSkipped;// Bytes skipped at start of stream (e.g. wrapper/header)
   mutable size_t ObjectSize; // 0 if unknown, set if wrapper seen or EOF reached
   mutable bool EOFReached;
 
-  // Fetch enough bytes such that Pos can be read or EOF is reached
-  // (i.e. BytesRead > Pos). Return true if Pos can be read.
-  // Unlike most of the functions in BitcodeReader, returns true on success.
-  // Most of the requests will be small, but we fetch at kChunkSize bytes
-  // at a time to avoid making too many potentially expensive GetBytes calls
+  // Fetch enough bytes such that Pos can be read (i.e. BytesRead >
+  // Pos). Returns true if Pos can be read.  Unlike most of the
+  // functions in BitcodeReader, returns true on success.  Most of the
+  // requests will be small, but we fetch at kChunkSize bytes at a
+  // time to avoid making too many potentially expensive GetBytes
+  // calls.
   bool fetchToPos(size_t Pos) const {
-    if (EOFReached)
-      return Pos < ObjectSize;
     while (Pos >= BytesRead) {
+      if (EOFReached)
+        return false;
       Bytes.resize(BytesRead + BytesSkipped + kChunkSize);
       size_t bytes = Streamer->GetBytes(&Bytes[BytesRead + BytesSkipped],
                                         kChunkSize);
       BytesRead += bytes;
-      if (bytes != kChunkSize) { // reached EOF/ran out of bytes
+      if (bytes == 0) { // reached EOF/ran out of bytes
+        if (ObjectSize == 0)
           ObjectSize = BytesRead;
         EOFReached = true;
-        break;
       }
     }
-    return Pos < BytesRead;
+    return !ObjectSize || Pos < ObjectSize;
   }
 
   StreamingMemoryObject(const StreamingMemoryObject&) = delete;
   void operator=(const StreamingMemoryObject&) = delete;
 };
 
 MemoryObject *getNonStreamedMemoryObject(
     const unsigned char *Start, const unsigned char *End);
 
 }
 #endif  // STREAMINGMEMORYOBJECT_H_

llvm/trunk/lib/Support/StreamingMemoryObject.cpp

 }
 } // anonymous namespace
 
 namespace llvm {
 // If the bitcode has a header, then its size is known, and we don't have to
 // block until we actually want to read it.
 bool StreamingMemoryObject::isValidAddress(uint64_t address) const {
   if (ObjectSize && address < ObjectSize) return true;
-    return fetchToPos(address);
 }
 
 uint64_t StreamingMemoryObject::getExtent() const {
   if (ObjectSize) return ObjectSize;
   size_t pos = BytesRead + kChunkSize;
   // keep fetching until we run out of bytes
   while (fetchToPos(pos)) pos += kChunkSize;
   return ObjectSize;
 }
 
 uint64_t StreamingMemoryObject::readBytes(uint8_t *Buf, uint64_t Size,
                                           uint64_t Address) const {
   fetchToPos(Address + Size - 1);
-  if (Address >= BytesRead)
+  // Note: For wrapped bitcode files will set ObjectSize after the
+  // first call to fetchToPos. In such cases, ObjectSize can be
+  // smaller than BytesRead.
+  size_t MaxAddress =
+      (ObjectSize && ObjectSize < BytesRead) ? ObjectSize : BytesRead;
+  if (Address >= MaxAddress)
     return 0;
 
   uint64_t End = Address + Size;
-  if (End > BytesRead)
-    End = BytesRead;
-  assert(static_cast<int64_t>(End - Address) >= 0);
+  if (End > MaxAddress)
+    End = MaxAddress;
+  assert(End >= Address);
   Size = End - Address;
   memcpy(Buf, &Bytes[Address + BytesSkipped], Size);
   return Size;
 }
 
 bool StreamingMemoryObject::dropLeadingBytes(size_t s) {
   if (BytesRead < s) return true;
   BytesSkipped = s;
   BytesRead -= s;
   return false;
 }
 
 void StreamingMemoryObject::setKnownObjectSize(size_t size) {
   ObjectSize = size;
   Bytes.reserve(size);
+  if (ObjectSize <= BytesRead)
+    EOFReached = true;
 }
 
 MemoryObject *getNonStreamedMemoryObject(const unsigned char *Start,
                                          const unsigned char *End) {
   return new RawMemoryObject(Start, End);
 }
 
 StreamingMemoryObject::StreamingMemoryObject(DataStreamer *streamer) :
   Bytes(kChunkSize), Streamer(streamer), BytesRead(0), BytesSkipped(0),
   ObjectSize(0), EOFReached(false) {
   BytesRead = streamer->GetBytes(&Bytes[0], kChunkSize);
 }
 }

llvm/trunk/unittests/Support/StreamingMemoryObject.cpp

 };
 }
 
 TEST(StreamingMemoryObject, Test) {
   auto *DS = new NullDataStreamer();
   StreamingMemoryObject O(DS);
   EXPECT_TRUE(O.isValidAddress(32 * 1024));
 }
+
+TEST(StreamingMemoryObject, TestSetKnownObjectSize) {
+  auto *DS = new NullDataStreamer();
+  StreamingMemoryObject O(DS);
+  uint8_t Buf[32];
+  EXPECT_EQ((uint64_t) 16, O.readBytes(Buf, 16, 0));
+  O.setKnownObjectSize(24);
+  EXPECT_EQ((uint64_t) 8, O.readBytes(Buf, 16, 16));
+}