readline(TFile, var string) segfaults if second argument is nil #564

Closed
rbehrends opened this Issue Aug 5, 2013 · 7 comments

Projects

None yet

6 participants

@rbehrends
Contributor

When the string parameter to readline is not initialized, the procedure segfaults. This occurs because it tries to reuse an existing string, but does not check if it has actually been initialized.

var f: TFile

discard open(f, "example.txt", fmRead)
while true:
  var line: string
  if not readLine(f, line):
    break
  echo line
close(f)
@reactormonk
Contributor

The compiler can't really check if a variable has been initialized, because nimrod as a language does allow null pointers. What would you have the compiler do? Check for nil at runtime?

@rbehrends
Contributor

The compiler shouldn't check it. The readLine implementation should. E.g.:

if line == nil:
  line = ""
else:
  setLen(line.string, 0) # reuse the buffer!
@dom96
Member
dom96 commented Aug 6, 2013

The docs for readLine state "line must not be nil!" which implies that this is intentional, i'm not sure why though. @Araq will know more.

@rbehrends
Contributor

@dom96: I suspect that it would be for performance reasons (though the gain is minimal, given that we're talking about a function likely dominated by other things, especially I/O, and branch prediction eliminating most of it).

The problem is that this is a somewhat fragile contract; it can be violated by accident in a form that testing does not uncover until the code is in production (e.g., if the line variable is initialized only along one code path).

@zah
Member
zah commented Aug 6, 2013

As discussed with @Araq on IRC, this behavior is likely to change in the future, when strings will be initialized to an empty string instead of nil. Another planned feature is not nil annotations for parameters that the compiler will be able to enforce at compile-time for reference types in situations similar to this.

@Varriount Varriount added the stdlib label May 24, 2014
@dom96 dom96 added the Showstopper label Jul 20, 2014
@Araq
Member
Araq commented Aug 11, 2014

This is in no way a showstopper.

@Araq Araq removed the Showstopper label Aug 11, 2014
@Varriount Varriount self-assigned this Nov 2, 2014
@dom96
Member
dom96 commented Aug 23, 2015

Blocked until not nil is ready.

@dom96 dom96 added the Blocked label Aug 23, 2015
@Araq Araq added a commit that closed this issue Oct 21, 2016
@Araq Araq fixes #564 e2eb9f8
@Araq Araq closed this in e2eb9f8 Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment