• Tom Lane's avatar
    Fix sloppy handling of corner-case errors in fd.c. · f97de05a
    Tom Lane authored
    Several places in fd.c had badly-thought-through handling of error returns
    from lseek() and close().  The fact that those would seldom fail on valid
    FDs is probably the reason we've not noticed this up to now; but if they
    did fail, we'd get quite confused.
    
    LruDelete and LruInsert actually just Assert'd that lseek never fails,
    which is pretty awful on its face.
    
    In LruDelete, we indeed can't throw an error, because that's likely to get
    called during error abort and so throwing an error would probably just lead
    to an infinite loop.  But by the same token, throwing an error from the
    close() right after that was ill-advised, not to mention that it would've
    left the LRU state corrupted since we'd already unlinked the VFD from the
    list.  I also noticed that really, most of the time, we should know the
    current seek position and it shouldn't be necessary to do an lseek here at
    all.  As patched, if we don't have a seek position and an lseek attempt
    doesn't give us one, we'll close the file but then subsequent re-open
    attempts will fail (except in the somewhat-unlikely case that a
    FileSeek(SEEK_SET) call comes between and allows us to re-establish a known
    target seek position).  This isn't great but it won't result in any state
    corruption.
    
    Meanwhile, having an Assert instead of an honest test in LruInsert is
    really dangerous: if that lseek failed, a subsequent read or write would
    read or write from the start of the file, not where the caller expected,
    leading to data corruption.
    
    In both LruDelete and FileClose, if close() fails, just LOG that and mark
    the VFD closed anyway.  Possibly leaking an FD is preferable to getting
    into an infinite loop or corrupting the VFD list.  Besides, as far as I can
    tell from the POSIX spec, it's unspecified whether or not the file has been
    closed, so treating it as still open could be the wrong thing anyhow.
    
    I also fixed a number of other places that were being sloppy about
    behaving correctly when the seekPos is unknown.
    
    Also, I changed FileSeek to return -1 with EINVAL for the cases where it
    detects a bad offset, rather than throwing a hard elog(ERROR).  It seemed
    pretty inconsistent that some bad-offset cases would get a failure return
    while others got elog(ERROR).  It was missing an offset validity check for
    the SEEK_CUR case on a closed file, too.
    
    Back-patch to all supported branches, since all this code is fundamentally
    identical in all of them.
    
    Discussion: https://postgr.es/m/2982.1487617365@sss.pgh.pa.us
    f97de05a
fd.c 79.9 KB