Re: [xml] Potential problem with 2.7.4

[Mike Hommey <mh glandium org>]

On Tue, Sep 15, 2009 at 11:22:45PM +0200, Daniel Veillard wrote:
> On Tue, Sep 15, 2009 at 10:49:23PM +0200, Mike Hommey wrote:
> > On Tue, Sep 15, 2009 at 10:09:15PM +0200, Mike Hommey wrote:
> [...]
> > diff --git a/xmlIO.c b/xmlIO.c
> > index c03ac43..1e7d213 100644
> > --- a/xmlIO.c
> > +++ b/xmlIO.c
> > @@ -3149,7 +3149,11 @@ xmlParserInputBufferGrow(xmlParserInputBufferPtr in, int len) {
> >       * Call the read method for this I/O type.
> >       */
> >      if (in->readcallback != NULL) {
> > -   res = in->readcallback(in->context, &buffer[0], len);
> > +        nbchars = 0;
> > +        while (len && (res = in->readcallback(in->context, &buffer[nbchars], len)) > 0) {
> > +           nbchars += res;
> > +           len -= res;
> > +        }
> >     if (res <= 0)
> >         in->readcallback = endOfInput;
> >      } else {
> > @@ -3160,7 +3164,7 @@ xmlParserInputBufferGrow(xmlParserInputBufferPtr in, int len) {
> >      if (res < 0) {
> >     return(-1);
> >      }
> > -    len = res;
> > +    len = nbchars;
> >      if (in->encoder != NULL) {
> >          unsigned int use;
>
> Hum, no I don't like that. The semantic of GROW is try to grow not
> wait until you get that amount of data. len is indicative as expressed
> in the comment and I prefer not try to guess how much stuff might break
> in subtle ways if you change that semantic. For example a Jabber client
> will probably deadlock because we are waiting for a buffer full while
> the server sent some kind of ping and is waiting for an answer which
> will never come because the client is still stuck trying to wait for
> more data instead of answering the ping.

I was not aware of this use of the API, it obviously would blatantly
fail.

> Your scheme work for things like reading from a file, but one should use
> the existing APIs and inkscape extension should just do that (the core
> inkscape parsing routine actually feed the parser properly).

What bothers me with the current fix is that somehow the code expects 35
bytes for the beginning of the buffer, yet doesn't care to make sure
that there be that amount of bytes. I haven't looked too deeply at the
code, but that sounds like this could lead to segfaults if some stupid
things are done with this API.

Surely, they should be using other APIs, but this one exists and is
public. Don't you think it would be better to avoid applications
shooting themselves in the foot ?

Mike