Bug 11411

Created attachment 10491 [details] [review]
Handle st.st_size correctly

The attached patch is only a partial fix for the first part of the issue to correct the handling of st.st_size.

Fixed in r4670 trunk.

I've noticed the your assumption about the buffer is incorrect.  There is no such a buffer overflow case as the allocated buffer is only used for a token.

Anyway thank for your report. The code you pointed out was totally brain damaged to allocate huge unused space.

No, it is really possible to cause a buffer overflow, if the file changes after you've determined its filesize because the code assumes no token will be larger than the file itself. This is a reasonable assumption, if and only if you're working with atomic objects or using atomic operations. That's not the case here.

If you look at nexttoken() in gtk/compose.c (the old version before your edit) you can see that there are two loops which only terminate if certain characters are found and otherwise keep on writing to the pre-allocated token buffer. As you basically you use getc() to fetch characters from the file, the originally determined filesize is nowhere taken into account. Thus in case of a growing file, you may very well run over the last element of the token buffer.

Note, I'm very well aware that this scenario is far from being a realistic security issue or the like. This was not the point of my report. The point was simply that this parsing algorithm was fundamentally flawed. If and only if you read the whole file into memory for parsing and ensured that the last character is a terminator, then this algorithm could be applied.

> No, it is really possible to cause a buffer overflow, if the file changes after
> you've determined its filesize because the code assumes no token will be larger
> than the file itself.

Ah.  That is possible.  Now, current code (r4706) checks the token size itself, so the problem is fixed I think.

Thanks for the comment.  It helps very much.