Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Totally broken code (and rotten logic) in snarf_blob() in alice_meta.epp [CORE1870] #2301

Open
firebird-automations opened this issue Apr 28, 2008 · 0 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: Claudio Valderrama C. (robocop)

Alex agreed privately that I can log this bug. For laziness, I'm copying my email to fb-devel here:

Look at snarf_blob() in alice_meta.epp, please. After the comment
// get the blob into the buffer, if it fits

Here we have a loop trying to read the blob's segments. The idea is that if the buffer isn't enough, buffer_length will become zero, so the second loop can read the full blob. I see reveral incredible bugs in this function. Maybe it's because I need even more sleep, so forgive me. I'm amazed this can work and this the reason I find strange that nobody found suspicious code; I would prefer that you say the code is correct and that I should reread it carefully.

1.- I moved "returned_length" as local var inside each loop, but this is misc change. It's only used to get the number of bytes read if the get_segment call succeeded.

2.- The condition in the first loop is moronic:
for (;;) {
if (ptr >= end)
break;
if (!(buffer_length = end - ptr))
break;
I can't imagine how the second condition can be true in my whole life, since the first condition rules it out. For me, this is simply an assignment and nothing else to check:
for (;;) {
if (ptr >= end)
break;
buffer_length = end - ptr;
In other words, this loop can never set buffer_length to zero. Am I crazy?

3.- Let's assume I simplify the code as shown above. This loop should exit with buffer_length > 0 in case of being able to read all the blob, then it will skip the second loop and will set buffer_length again to zero, so the caller can see the provided buffer was enough. Otherwise, the first loop will quit with buffer_length being zero to indicate it failed to read the full blob and the second loop will continue reading (note it continues reading the blob where the first call left it) simply to get the full blob length and report to the caller the dynamic buffer size that should be allocated (the first call is done with a stack-based buffer).

4.- As I can see, there's no way the first loop can set buffer_length to zero. A naive attempt would be
for (;;) {
if (ptr > end) // instead of >=
break;
if (!(buffer_length = end - ptr))
break;
In case ptr == end, we quit with buffer_length being zero. However, this code is unable to distinguish the condition when the buffer is not enough from the condition when all the blob was read and we just reached end of buffer. Yes, there's little change that blob's length and buffer's length match, but why should we leave potential failure enable? Suggestion to rewrite this crap, please.

5.- Ok, let's assume the first loop is fixed and now indicates correctly when more buffer is needed. Then the second loop will execute. This second loop will start with buffer_length being zero. Remember this var originally contained the incoming buffer's size? We have lost its value. Now the second loop does something incredible. Can you teach me how this second loop isn't infinite? The get_segment call is told the incoming buffer's length is buffer_length, but this is zero!
if (!buffer_length)
{
for (;;) {
USHORT returned_length;
ISC_STATUS status = isc_get_segment(gds_status, &blob, &returned_length,
buffer_length, buffer);
if (status && status != isc_segment)
break;
buffer_length += returned_length;
}
}
Therefore, nothing happens here. I checked BLB_get_segment and passing a zero-length buffer doesn't seem to trigger an error condition. In jrd.cpp, GDS_GET_SEGMENT passes buffer_length unchecked to BLB_get_segment. The code will set returned_length to zero, hence buffer_length is never incremented. We are doing
for (;;) {
USHORT returned_length;
ISC_STATUS status = isc_get_segment(gds_status, &blob, &returned_length,
ZERO, buffer);
if (status && status != isc_segment) -> always isc_segment
break;
buffer_length += ZERO; // returned_length will be zero
}
The returned condition will be isc_segment always and thus the loop runs until doomsday. Thanks to the bug in the second loop, the test
if (!buffer_length)
is always false and this loop never gets a change to run.

6.- One solution is to stop messing with buffer_length and have a bool var. Ok, let's assume "need_more" is true and buffer_length wasn't changed before. It will contain 1024 as indicated by the caller the first time. Now we have
if (need_more)
{
for (;;) {
USHORT returned_length;
ISC_STATUS status = isc_get_segment(gds_status, &blob, &returned_length,
buffer_length, buffer);
if (status && status != isc_segment)
break;
buffer_length += returned_length;
}
}
else
buffer_length = 0;
This damn code is still wrong and worse than before! At least today it doesn't execute. We have changed a potential infinite loop by a possible crash: the first call will tell get_segment() we have buffer_length available, but if we continue iterating, the second call will tell get_segment we have
buffer_length + returned length available. In case the segment was enough big, it would simply cause a buffer overrun. We can't change the input length because it's the length of the buffer! Another var is needed.

7.- The code that calls snarf_blob is in get_description. If this function is fooled believing the buffer wasn't enough, will allocate enough buffer and call snarf_blob again (I guess the dynamic buffer's length will be wrong due to (4), hence it will crash anyway the second call to snarf_blob).

At this time I'm feeling mad. I just wanted to fix a couple of misc issues in alice (for example, avoid get_description reading beyond the buffer in its while loop) and now I'm close to start pulling my hair.
:-)

How is snarf_blob() able to do its duty? My answers are:
- this code only runs for fixing two phase txns. Rarely used.
- this code was written without reading it again and the morons that did it couldn't be helped by const input vars because they didn't exist in C and QA was either dumb or inexistent. Typical 1980-1990 code that let the operating system detect program's fatal behavior and shut down it ("the user never does something unexpected"). Since the code was done in the US, "in God we trust" that the critical condition won't be reached.
- apparently the stack-based buffer is more than enough at 1024 bytes. I would like a way to produce artificially bigger information to call get_description() since I doubt MET_get_transaction will pull enough from TRA.RDB$TRANSACTION_DESCRIPTION. This field probably has little info. Otherwise we would see an egregious crash.

This is an example of logically const incoming parameters being reused for other objectives and thus destroying the logic (and crashing the server if the fix is naive). I know some people hate it, but...make the incoming params const if it's possible and it makes sense, folks. This is also an example of someone trying to appear cleverer by minimizing the number of needed variables to implement complex logic. Reusing vars here is a disaster. Finally, we are ironically saved from an infinite loop or a royal crash simply because the bug in the first loop prevents the second loop from burning the server. If that code was written for a tyrant, the programmer would have his head chopped off.
:-)

Now someone with better ideas than me, please think a way to rewrite this crap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment