pg_restore loops forever past EOF for corrupt custom archive files

This patch modifies the ReadStr function in pg_backup_archiver.c to validate
the result of *AH->ReadBufPtr matches the value of l. The resulting error
is:

pg_restore: [archiver] expected 410 bytes, only got 275 bytes

"Chad Wagner" <chad(dot)wagner(at)gmail(dot)com> writes:
> This patch modifies the ReadStr function in pg_backup_archiver.c to validate
> the result of *AH->ReadBufPtr matches the value of l.

If we're trying to defend against premature EOF, this hardly seems like
a sufficient patch.

regards, tom lane

On 8/5/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> "Chad Wagner" <chad(dot)wagner(at)gmail(dot)com> writes:
> > This patch modifies the ReadStr function in pg_backup_archiver.c to
> validate
> > the result of *AH->ReadBufPtr matches the value of l.
>
> If we're trying to defend against premature EOF, this hardly seems like
> a sufficient patch.

I agree, but it is better than nothing. If you have some suggestions or
other areas of the pg_restore code that I should take a look at then I don't
mind doing so.

The patch at least raises awareness to this problem.

"Chad Wagner" <chad(dot)wagner(at)gmail(dot)com> writes:
> On 8/5/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If we're trying to defend against premature EOF, this hardly seems like
>> a sufficient patch.

> I agree, but it is better than nothing. If you have some suggestions or
> other areas of the pg_restore code that I should take a look at then I don't
> mind doing so.

I looked at this a little bit. The various implementations of
ReadBytePtr all seem to think they should return EOF rather than failing
at EOF, but there is not any call site whatsoever that is either making
use of this to handle an expected EOF case, nor testing for failure.
There are quite a few call sites and they will all fail to behave sanely
for early EOF. So I propose that we make the ReadByte subroutines
die_horribly() on EOF instead of returning EOF.

I see only two calls of ReadBufPtr, the one Chad fingers and the one
in ReadHead(), both of which need to be checking the read length.
Alternatively, we could change the API of ReadBufPtr to say that the
error check should be done inside the subroutine. That feels like it
might be a bad choice though --- there would then not be *any* way of
reading that wouldn't fail on early EOF, and someday we might want one.

So my proposal is to error out on EOF inside the subroutine in the
ReadByte case, but make the callers check it in the ReadBuf case,
even though this isn't totally consistent. Comments?

regards, tom lane