Re: PNM bugfix doesn't help

becka@rz.uni-duesseldorf.de
Thu, 10 Dec 1998 13:48:40 +0100 (MET)

Hi !

> > If it is broken, it is probably largely my fault.
> Good, I'm talking to the right guy.

Seems not. I haven't touched it for some time, and the bug below is
quite surely not (c) by me.

> > /* Suck in as much of the file as possible, since it's already in the
> > correct format. */
> > len = fread (data, 1, max_length, infile);

> Well, the code you have here is totally different to the code in the
> SANE 1.0 source tarball I have. Sigh...

ARGH ! I checked against 0.76, thinking noone has touched the thing
sinc then.

> /* Suck in as much of the file as possible, since it's already in the
> correct format. */
> char *buf;
> int doread = parms.bytes_per_line;
> if (read_lines >= bry)
> return SANE_STATUS_EOF;
> buf = alloca(doread);
> len = fread (buf, 1, doread, infile);

Hmm - this is simply wrong as far as I can tell.

I'm just now downloading a copy to be able to make better comments. Sorry
for the inconvenience.

Who did that changes ?

Hmmm: Changes written by G.ran Thyni <goran@bildbasen.se> but I'm not sure
he's responsible for those changes.

Anyway: Whoever does changes - could you make sure they do not break anything ?
The PNM backend is a reference implementation. Not a playground.

It is totally irrelevant, what it does. It is just there to look at and see
how things are done.

> > Now let's have a look at the other branch ...
> [snipped, I'm sure both cases are broken, because the PNM backend is
> totally borked, and I'd like to see the author fix it, or rewrite it]

*grin* Me too. I'll take care of it.

It can't be, that the reference implementation backend is broken.

> Tested, doesn't fix it (because we're in the other branch by this point)

Yes, but that doesn't matter any more, as the code is too much different from
what I analyzed.

> Can't tell if it helps when Grayify/Three-pass are on, because they always
> return total gibberish anyway. Make the easy case work first?

Yep. I'll make that work here some way and send diffs.

> > > If the PNM device has more data to send, it just writes past the end
> > > of the buffer,
> > Hey - it's not that bad. Just some overly clever code, which misbehaves
> > when used in a somewhat unusual way. Who didn't make such a mistake once
> > upon a time ?

> The SANE standard says I can pass any value for max_length, the PNM
> backend assumes I will always pass >= sizeof(one scanline),

I didn't know it was _THAT_ broken in 1.0. Now I understand you being upset.

The code I analyzed from 0.76 just broke in rare occasions, while the code
snippet you sent suggest that it breaks _always_ ... !

> which is a totally bogus assumption. I notice on the list that at least one
> other backend makes such bogus assumptions.

Yes. This is simply silly. The reason for having that maxlength parameter is
to _avoid_ buffer overflows.

> Perhaps I should write a test frontend, that behaves badly to crash
> wrong-headed backends.

Yes. Please !

Use sane-read with widely varying max_sizes and adjust them up _and_ down
in one scan.

That would have crashed the old PNM backend as well.

> If I did this would the SANE developers agree to not allow distribution
> of backends that can't pass the test?
> [They are non-conforming by SANE standard]

Interesting idea. We should consider that.

> Alternatively the standard could be revised to say "Backends reserve
> the right to copy any amount of data into the buffer, even if this
> causes overflows or the end of world civilisation, so there."
*grin*. We should call that plan B :-).

CU, Andy

-- 
Andreas Beck              |  Email :  <Andreas.Beck@ggi-project.org>

--
Source code, list archive, and docs: http://www.mostang.com/sane/
To unsubscribe: echo unsubscribe sane-devel | mail majordomo@mostang.com