First of all it ignores that st.st_size is off_t which is usually wider than "long" which can result in allocating to little memory as the cast will truncate the value. If the file is 4 GiB + 1 byte large, only 1 byte will be allocated resulting in a buffer overflow. However, even with proper checks in place the file can grow after fstat() and the initial memory allocation might be insufficient resulting in a buffer overflow again. The parsing code should be changed to take the size of the allocated buffer into account.
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.