chapel-lang / chapel

a Productive Parallel Programming Language
https://chapel-lang.org
Other
1.78k stars 418 forks source link

readBinary should throw an error when reading an array beyond a fileReader's region #23716

Open benharsh opened 10 months ago

benharsh commented 10 months ago

The following program fails to throw an error when reading an array that is too large for the designated file region:

use IO;

proc main() {
  const D = {1..100};
  const SENTINEL = 5;

  var f = openMemFile();
  {
    var A : [D] int = SENTINEL;
    var w = f.writer(serializer=new binarySerializer());
    w.write(A);
  }

  try {
    var B : [D] int = 1;
    var r = f.reader(locking=false, deserializer=new binaryDeserializer());
    const seekRange = 0..#(50 * 8);
    r.seek(seekRange);
    r.readBinary(B);
    if && reduce (B==SENTINEL) then writeln("SUCCESS");
    else writeln("FAILURE");
  } catch e {
    writeln(e.message());
  }

}

Throwing an error here is not only appropriate, but can help users to diagnose surprising off-by-one issues where the program silently fails to read the last element.

This issue also manifests for the version of readBinary that takes a c_ptr, and for fileReader.read when using a binaryDeserializer.

This issue is less clear for the c_ptr case, which also accepts a max number of bytes to read, and so it's not immediately obvious the caller wanted to read a specific number of bytes.

A possible related question is "should reading past a file region manifest as an EOF, or some other kind of error?".

Related futures:

bradcray commented 9 months ago

I'm throwing a "user issue" label on here since it was motivated by a case that PSath ran afoul of.

benharsh commented 9 months ago

That sounds good to me. For anyone else following along, my initial investigation into this issue spawned more questions, and made me realize that this was more complex than I had initially expected. I'll be working on a proposal to address the problem in this issue, and some related problems.

benharsh commented 9 months ago

https://github.com/chapel-lang/chapel/pull/23958 fixes a couple of bugs, but does not address some of the core usability issues (i.e. checking return value of fileReader.read), so at the moment it is still somewhat easy to silently fail to notice that an array wasn't fully read.