Event removal bug

[jacobalberty]

Event's are still giving me issues, firing works great now but removal seems to have issues, I threw in a few cout lines into removeEvent(char *Event) to check values and I'm seeing some values that are way off for the memcpy. I'm unable to reproduce the issues using nodeunit at this time, but in production I end up with a tale_length of 18446744073709551549 This seems to be because in this crash the sz value ended up being 87 while blength was 35, resulting in a negative value that was cast to size_t for new_length

I'm still trying to produce a test case but for now here's my modified removeEvent that I used to get those values, and the output for one crash. The issue seems to occur after adding two events then removing the second event. It happens every time when I run my nodejs code but nodeunit I haven't yet been able to track down a good test case.

void event_block::removeEvent(char *Event)
    { 
      int idx = hasEvent(Event); 

      if(idx<0) return;
      free(event_names[idx]);
      for(int i = idx;i < count;i++) event_names[i] = event_names[i+1];
      count--;

      // realocate buffers
      char *buf = (char*) event_buffer + 1;
      char *rb  = (char*) result_buffer + 1; 
      while(idx)
      { 
        buf = buf + (*(buf)) + 4; buf++;
        rb  = rb + (*(buf)) + 4; buf++;
        idx--;
      }
      char sz = (*buf) + 5;
      std::cout << "Got new sz: " << (int)sz << std::endl;
      size_t new_length = blength - sz;
      size_t tale_length = blength - (buf - (char*)event_buffer) - sz;    
      std::cout << "blength: " << blength << std::endl;
      std::cout << "new_length: " << new_length << " tale_length: " << tale_length << std::endl;
      if(tale_length){
        memcpy(buf, buf+sz, tale_length);
        memcpy(rb, rb+sz, tale_length);
      }    
      std::cout << "did the memcpy: " << (int)tale_length << std::endl;
      if(new_length==1) new_length = 0;

      event_buffer = (ISC_UCHAR*) realloc(event_buffer, new_length); 
      result_buffer = (ISC_UCHAR*) realloc(result_buffer, new_length);             
      blength = (short) new_length;

    }

events_1 | Got new sz: 87 events_1 | blength: 35 events_1 | new_length: 18446744073709551564 tale_length: 18446744073709551549

[jacobalberty]

Edit: I added a testcase to my own repository you can browse the code at https://github.com/jacobalberty/node-firebird-libfbclient/blob/testmultieventremove/tests/def/test-events.js or clone my repository and checkout the branch testmultieventremove

Well that didn't take long, I modified addanddelete test and it causes a segfault, if you need me to I can figure out nodeunit enough to make this its own test case but for now heres the modified addanddelete that crashes

exports.AddAndDelete = function(test){

  test.expect(2);
  Init();
  var conn = Connect();
  test.ok(conn.connected,"Connected to database");
  var called = false;
  conn.on("fbevent", function(event,count){
    //console.log("->"+event+" "+count);
    called = true;
  });
  var eN = 'eventName';
  var eN2 = 'eventName2';
  conn.addFBevent(eN);
  conn.addFBevent(eN2);
  conn.deleteFBevent(eN2);
  GenEvent(eN);

  setTimeout(function(){
       test.ok(!called,"Event not called");
       conn.disconnect();
       CleanUp();
       test.done();
  }, 1000);

}

[jacobalberty]

In the removeEvent() function what is this bit of code supposed to be doing?

      while(idx)
      { 
        buf = buf + (*(buf)) + 4; buf++;
        rb  = rb + (*(buf)) + 4; buf++;
        idx--;
      }

If I comment it out the new AddAndDeleteMultiSimple but not the newly created AddAndDeleteMultiComplex from my own fork seen at https://github.com/jacobalberty/node-firebird-libfbclient/blob/testmultieventremove/tests/def/test-events.js

Here's my git diff

diff --git a/src/fb-bindings-eventblock.cc b/src/fb-bindings-eventblock.cc
index 1fc6523..764bdd9 100644
--- a/src/fb-bindings-eventblock.cc
+++ b/src/fb-bindings-eventblock.cc
@@ -302,12 +302,14 @@ void event_block::removeEvent(char *Event)
       // realocate buffers
       char *buf = (char*) event_buffer + 1;
       char *rb  = (char*) result_buffer + 1; 
+/*
       while(idx)
       { 
         buf = buf + (*(buf)) + 4; buf++;
         rb  = rb + (*(buf)) + 4;  #buf++;
         idx--;
       }
+*/
       char sz = (*buf) + 5;
       size_t new_length = blength - sz;
       size_t tale_length = blength - (buf - (char*)event_buffer) - sz;    

[jacobalberty]

It looks like
rb = rb + (*(buf)) + 4; buf++; was supposed to read rb = rb + (*(buf)) + 4; rb++;

I've submitted a pr with the test case and the fix, I'm testing with my nodejs event server now It seems there may still be another crash, possibly still in removeEvent, but I haven't figured out a test case for it yet.

[jacobalberty]

Made AddAndDeleteMult iComplex even more complex and it triggers what may be a new crash, it gets further along and makes it out of that function before segfaulting

https://github.com/jacobalberty/node-firebird-libfbclient/blob/multieventcont/tests/def/test-events.js contains the new test case.

[jacobalberty]

It may be time to rewrite fb-bindings-eventhandler to use a C++ vector like ibpp does but I think i finally have a some idea whats going on. in RegEvent(event_block* rootp, char Event, Connection aconn, isc_db_handle db) it's creating a new event block every 15 events, which is appropriate to do as thats a firebird requirement. but it seems after it creates the new block the next time its called something must have been initialized wrong and it ends up creating a new event block every other event because it sees the count as 15 again.

I'll keep trying to trace down the issue to get a patch but I think the best solution may be to do a complete rewrite and get rid of all the C type code, I'm just not confident enough in my C++ to do that at this time.

[xdenser]

For me it works - it creates new event block for each 15 events.

[jacobalberty]

Yeah I think I was on a red herring with that it looks like the event blocks are just getting corrupted. Do a dump_bufs at the end of removeevents . Looks bad pointer math somewhere I haven't been able to pin down yet

On Sat, Jan 28, 2017, 9:32 AM Denys Khanzhiyev notifications@github.com wrote:

For me it works - it creates new event block for each 15 events.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/xdenser/node-firebird-libfbclient/issues/70#issuecomment-275854693, or mute the thread https://github.com/notifications/unsubscribe-auth/AB42gisiIDlkVSHmlYDuU0np5LSrMbo5ks5rW19ugaJpZM4LttFZ .

[xdenser]

actually event removal code had another bug, see last commit

[jacobalberty]

Found one last bug I'm setting up a pull request now. It's tested with hundreds of events showing no issues.

[jacobalberty]

72 has been opened I am building to test on my actual real life code now but it definitely passes the complex tests now. I merged your other removeEvent fix into it as well so it should be clean to apply.