Re: [PATCH 6/15] hypervisor console driver for Celleb

[linas@austin.ibm.com (Linas Vepstas)]

On Thu, Dec 14, 2006 at 10:42:16AM +0900, Ishizaki Kou wrote:
> Linas-san,
> 
> Thanks for your comment.
> 
> > On Tue, Dec 12, 2006 at 12:31:29PM +0900, Ishizaki Kou wrote:
> > > +
> > > +static int hvc_beat_get_chars(uint32_t vtermno, char *buf, int cnt)
> > > +{
> > > +	unsigned long kb[2];
> > > +	unsigned long got;
> > > +
> > > +	if (beat_get_term_char(vtermno, &got, &kb[0], &kb[1]) == 0) {
> > > +	   memcpy(buf, kb, got);
> > > +		       return got;
> 
> > This seems to completely ignore "cnt". Thus, I presume that
> > beat_get_term_char might return more chars than there is room for in buf,
> > thus corrupting something, somewhere.
> 
> This depends "beat_get_term_char" returns only one character at once
> (for now), and assumes cnt > 0. This assumption will reduce code for
> now.

But it will break in the future. If new firmware is released for beat,
that returns more than one char, then it will silently corrupt old kernels
on rare occasions, making the problem hard to find. What's more, the 
firmware people might forget to tell you about thier change, and
so you won't submit a matching update to the linux kernel. (Or you may
have a new job by then, or have lost interest/got bored, etc.)
Years later, someone will finally debug the problem, after a long and
difficult search, and they'll curse this code.  Better do it right, now.

> > > +static int hvc_beat_put_chars(uint32_t vtermno, const char *buf, int cnt)
> > > +{
> > > +	unsigned long kb[2];
> > > +
> > > +	memcpy(kb, buf, sizeof(kb));
> > > +	beat_put_term_char(vtermno, cnt, kb[0], kb[1]);
> > > +	return cnt;
> > > +}
> 
> > I can't imagine how this can possibly work. 
> > What if "cnt" is greater than 8?
> 
> This routine assumes that 0 <= cnt <= 16, that is already checked by
> caller. (Note that "unsigned long" is 8 bytes long at ppc64)

Actually, its N_OUTBUF which is currently defined to be 16. However,
that may someday change, in which case you'd have a bug, again.

--linas