Crash at sending of 6th message

[bvandergiessen]

The first 5 messages are sent without problems, but at the 6th the program completely crashes - not in the way a Node.js program normally exits with an error, but it seems to be the interpreter itself crashing:

node: malloc.c:2372: sysmalloc: Assertion `(old_top == (((mbinptr) (((char *) &((av)->bins[((1) - 1) * 2])) - __builtin_offsetof (struct malloc_chunk, fd)))) && old_size == 0) || ((unsigned long) (old_size) >= (unsigned long)((((__builtin_offsetof (struct malloc_chunk, fd_nextsize))+((2 *(sizeof(size_t))) - 1)) & ~((2 *(sizeof(size_t))) - 1))) && ((old_top)->size & 0x1) && ((unsigned long) old_end & pagemask) == 0)' failed.
Aborted

This is under Node v7.9.0 on a Raspberry Pi 3 running Raspbian GNU/Linux 8.0 (jessie).

The behavior is reproducible: it's always at the 6th message. The first five arrive correctly. I think my code is according to the instructions: at the start of the program it does

var svmq = require('svmq');
var inqueue = new svmq(55809);
var sendarray = new Uint8Array(16);

and after that, each time a message must be sent, sendarray is populated with the right data, after which this is run:

inqueue.push(Buffer.from(sendarray), {type: 3}, (err) => {
    		if (err) console.log('Error: '+err);
    		else console.log('Command put on the message queue');
    	});

[jhead]

Hey there, thanks for the report. Have you tried the same code on any other environments?
i.e. different Node versions, different OS, etc.

I'll look into this as best I can. Any additional troubleshooting/info on your end would be appreciated as well.

[bvandergiessen]

Hi, thanks for your quick response. I haven't tried on other platforms, but I suppose the crash wouldn't happen there, since what I'm doing is standard usage (my code is basically copied from the instructions). Rather than something unusual I'm trying to do, the problem might be something on this platform.

Can I help with further data and/or further experiments? E.g. trying with a modified version of your C++ code, collecting certain debugging data etc.

[bvandergiessen]

Some more interesting facts have emerged; I hope they can help you find the problem.

1. The number of bytes per IPC message matters
2. The content of the message matters.

Here's some testing code I used:

// tests/ipctest.js
// test Inter-Proces Communication

const svmq = require('svmq');
var inqueue = new svmq(55809);

var sendarray = new Uint8Array(10);
sendarray.fill(0);
sendarray[1]=97;
sendarray[0]=98;
sendarray[8]=16;

var n = 0;

setInterval(function() {
	inqueue.push(Buffer.from(sendarray), (err) => {
		n++;
		console.log('Message '+n.toString()+' pushed');
	})
}, 1000);

This sends a message every second. In the form above, it doesn't crash. But:

• If I change the size of the message to 11 bytes (using Uint8Array(11) instead of (10)), it crashes after sending 3 messages
• If I populate the 10-byte message with only 0's (leaving out the three statements sendarray[1]=97; etc.), it crashes after sending 11 messages.
• If I change the size to 11 bytes but also populate bytes 2, 9 and 10 with nonzero values, it doesn't crash (at least not in the first 250 messages).

Again, these behaviors are reproducible: it's 3 messages every time for the first situation, and 11 for the second.

Does this give any clues as to the cause of the problem?

[bvandergiessen]

As an additional test, I tried the same script on a fresh install of Raspbian, but with an older version of Node.js: v4.8.2. With 11-byte messages (Uint8Array(11)), it crashes after sending 3 messages, just as it does on Node v7.9.0.

[bvandergiessen]

Would rebuilding the msg.node executable with the --debug option be useful? How could I then get useful output from it?

[bvandergiessen]

The crash always happens at one place in the functions.cc file, in line 46:

   msgbuf *message = new msgbuf;

It clearly has something to do with memory management (as the error message already reveals). For instance, if I add a line after line 52 like

   delete message;

the crashes occur under different circumstances. The two cases mentioned above don't crash anymore, but a new crash is introduced e.g. if I send an 11-byte message consisting of only zeros:

*** Error in `node': free(): invalid next size (fast): 0x72a00468 ***

which happens at the point where the new delete statement is. According to some web pages, this happens when you try to free memory that has already been freed.

Could the problem be to do with asynchronous operation? The code uses an AsyncQueueWorker to start the process. So perhaps it doesn't free up its memory in one case, and frees it up too soon in the other case. What do you think?

[jhead]

Hey @bvandergiessen thanks for your patience and help debugging. I fired up Valgrind and found a couple memory management issues. I'm admittedly not very familiar with C/C++ so bear with me.

I pushed a commit - fb6eb27 - on a separate branch: issues/1
Please try those changes and see if the issue goes away.

For what it's worth, I wasn't able to reproduce the crashes on Debian and Node v7.10. I'm wondering if it's something specific to Raspian. In any case, the poor memory management was there so I'm hoping to fix that regardless.

[bvandergiessen]

So far this seems to have stopped the crashes.
Hard to understand fully, since it just changes C++ new and delete to C malloc and free, but I guess that might do the job (I don't really know how memory management works in C++).

Thanks, I'll do some more testing tomorrow and if no further crashes occur I'll close the issue.

[jhead]

Glad to hear, it was a shot in the dark. I'm not entirely sure why either given my minimal understanding of memory management in C/C++ as well. I grabbed the malloc line from here, which shows what I assume is the correct way to dynamically allocate enough memory based on the incoming buffer size. Valgrind shows that there was a memory leak which is fixed now, so I'm going to assume I was doing something incorrectly before. Let me know if you notice any more crashes or unexpected behavior.

I'll probably make a couple small adjustments, add some basic unit tests, and merge the changes into master later this week. I wrote this about a year ago and haven't really touched it since, so it's taking me a little bit to get reacquainted. Once the fix is published to npm, I'll let you know.

Out of curiosity, what are you using this library for?

[bvandergiessen]

Thinking a bit more, the difference of course is that the new statement allocated just the 'official' amount of memory, with just 1 byte for the message field of the struct. The malloc statement that's there now actually allocates more memory: enough for the entire message. That explains why the memory overflows occurred in the old version.

I'd expect that in the other class, ReceiveMessageWorker, the same problem might occur: not enough memory may have been reserved to store the incoming message. In addition, the current version of the code, with message = new msgbuf; (line 87 in the new branch), doesn't comply with the documentation: the second argument of msgrcv should be a pointer to a msgbuf struct, instead of a msgbuf struct itself. So perhaps you can make it work by allocating bufferLength+4 bytes to a struct msgbuf * variable and pass that into the msgrcv as the second argument.

I'm working on a Node.js program that helps manage lights (turning them on and off, dimming, reacting to presence detection sensors). The direct contact with the lights is managed by a C library that runs in the background. In an older version, the Node.js program communicated with the C program through a FIFO, but I like the System V message queue much better because it doesn't ever get blocked when different programs try to write to it or when a program hasn't had time to read the messages yet.

[bvandergiessen]

Attached is a patch of functions.cc (starting from the version in the new branch) that embodies my remarks above, as well as some other thoughts:

• puts the sizeof(msgbuf) in a constant so it doesn't have to be calculated every time
• corrects the type of dataLength
• does memory allocation the same way in the ReceiveMessageWorker class as you did in the SendMessageWorker class, and makes message a pointer
• casts info[2] as a long just in case the system doesn't have 32-bit long type (some systems use 64 bits).

I haven't seen any crashes yet, so I'll close the issue.
Attachment: patch.txt