Thrift-Swift icon indicating copy to clipboard operation
Thrift-Swift copied to clipboard

Bug in TFileTransport.read(size:)?

Open kasei opened this issue 7 years ago • 3 comments

Hello. I'm running into an issue trying to use TFileTransport. Up front, let me say that this might be a mistake on my part as I'm new to both Thrift-Swift and Thrift in general. However, in trying to read an existing thrift file using swift code, I'm seeing a crash in TFileTransport.swift where a Data buffer is set up using:

var read = Data(capacity: size)

and then used:

read[position] = nextChar

There's no operation that changes the Data object's actual size before that write, so I believe what's going on here is that while the object has the capacity for size bytes, it's count is still zero when the write is attempted. If I've understood the issue correctly, this could be fixed by creating the Data object as a fixed-sized buffer of zeroed bytes instead of just hinting at the capacity:

var read = Data(count: size)

For context, here's how I'm calling into the thrift code:

guard let transport = try? TFileTransport(filename: filename) else { fatalError() }
try transport.open()
let proto = TBinaryProtocol(on: transport)
let d = try? MyThriftStruct.read(from: proto)

kasei avatar Mar 09 '17 22:03 kasei

Oops. That actually won't work because of the while conditional read.count < size. Perhaps instead of writing directly to a Data index like read[position], the UInt8 value could be appended to the empty Data object instead...?

kasei avatar Mar 09 '17 23:03 kasei

I think it'll work if the object is constructed the same as Data(capacity: size), but within the read loop, it is filled with:

read.append(nextChar)

kasei avatar Mar 09 '17 23:03 kasei

Hmm good catch, I've not tested the TFileTransport extensively (just barely) Could you replace the read method with this one and let me know if it fixes your issues? After a quick test it seems that index addressing doesn't play nice with Data(capacity:) because it just reserved space, instead of fully allocating it to the Data object, so while it has n-bytes capacity to write to, it technically only has 0 bytes (reported by count), and index replacements on Data since its a value type use replaceBytesInRange:withBytes, since the range is {0,0} that obviously doesn't work well does it?

  public func read(size: Int) throws -> Data {
    // set up read buffer, position 0
    var read = Data(capacity: size)
    
    // read character buffer
    var nextChar: UInt8 = 0
    
    // continue until we've read size bytes
    while read.count < size {
      if fread(&nextChar, 1, 1, self.fileHandle) == 1 {
        read.append(nextChar)
      } else {
        throw TTransportError(error: .endOfFile)
      }
    }
    return read
  }

apocolipse avatar Jun 07 '17 18:06 apocolipse