FreeTDS Development Group

["Eric Deutsch" <edeutsch AT systemsbiology.org>]

> -----Original Message-----
> From: Lowden, James K [mailto:LowdenJK AT bernstein.com]
> Sent: Tuesday, July 09, 2002 8:13 AM
> To: TDS Development Group
> Subject: [freetds] RE: Losing last character of TEXT fields
> 
> > > @@ -115,18 +116,20 @@
> > >
> > >     switch(desttype) {
> > >        case SYBTEXT:
> > > -      case SYBCHAR:
> > >          cplen = srclen > destlen ? destlen : srclen;
> > >           memcpy(dest, src, cplen);
> > > -         dest[cplen-1] = '\0';
> > > +         /* 2001-06-15 Deutsch changed [cplen-1] to [cplen] */
> > > +         dest[cplen] = '\0';
> > >           return strlen(dest);
> > > -      case SYBBINARY:
> > > +      case SYBCHAR:
> > >          cplen = srclen > destlen ? destlen : srclen;
> > >           memcpy(dest, src, cplen);
> > > -         return cplen;
> > > +               dest[cplen]='\0';
> > > +         return strlen(dest);
> > >     }
> > >
> > I'm not agree. I prefer loosing a character to a buffer overflow.
> > Perhaps seem that definition of srclen and destlen is different.
> > srclen is the len of input string without counting terminator
> > (there is no

correct.

> > terminator) while destlen if the len of the output buffer
> > counting needed terminator.
> > Is that right ?

destlen appears to be the size of the buffer that we have to work with.
If I set TEXTSIZE to 65536 in freetds.conf, I find that destlen is 65536
in this bit of code.

> > Then cplen should be the minimun of srclen and (destlen-1)

IF we need to have it \0 terminated, I totally agree.

> > That for read a varchar(255) we must pass a 256 char buffer.

I think this is right, but I'm not sure that dest[] needs to be \0
terminated.

> I think you're right, Frediano; the code is still broken.
> 
> We shouldn't null-terminate unless destlen is -1.  If the destination
> buffer
> is too small, we should call the error handler and return -1.  So say
the
> docs.  Eric?

Errr.. I think you may be right.  I changed my code to now read:

      case SYBTEXT:
      case SYBCHAR:
         cplen = srclen > destlen ? destlen : srclen;
         memcpy(dest, src, cplen);
         return cplen;
      case SYBBINARY:
         cplen = srclen > destlen ? destlen : srclen;
         memcpy(dest, src, cplen);
         return cplen;

and it seems to work fine.  So perhaps the real question is, why were we
ever fooling around with \0 termination in the first place?

I set my TEXTSIZE to something very small (10) and this code ran fine
under my setup.  SQL Server only sent the first 10 characters of my TEXT
field.  The dest[] buffer was only 10.  The memcpy went fine.  There was
no bother with \0 and my resultset looked fine.

So, I'd be happy with the above code.  It's simple, it's clean.  I only
worry that this may be reversing some fix that someone did long before
me to remedy some problem with an unterminated dest[].

So, where does this leave us?  A bold step would be to change the code
to:

      case SYBTEXT:
      case SYBCHAR:
      case SYBBINARY:
         cplen = srclen > destlen ? destlen : srclen;
         memcpy(dest, src, cplen);
         return cplen;

and see who can demonstrate a problem with it.  I can't.  And then when
we find that problem, we can document it.

If we're squeamish about doing this so close to 0.60, I'd be happy with:

         cplen = srclen > destlen ? (destlen-1) : srclen;
         memcpy(dest, src, cplen);
         dest[cplen] = '\0';
         return cplen;

which acknowledges that someone out there might want a \0 terminated
dest[], fixes my problem, and seems to be safe.  Its only side effect is
that the maximum size is actually TEXTSIZE - 1 instead of TEXTSIZE.

Wha'd'y'all think?

Thanks!
Eric


> 
> --jkl
> 
> ---
> You are currently subscribed to freetds as:
[edeutsch AT systemsbiology.org]
> To unsubscribe, forward this message to leave-freetds-
> 127899P AT franklin.oit.unc.edu