Lmms: Memcpy on a null pointer

[shlyakpavel]

https://github.com/LMMS/lmms/blob/7f0593c601a7f48fda193139f2d6bb3d76f10384/plugins/MidiImport/portsmf/allegro.cpp#L57-L58
As we see on line 58, atom can be a null pointer.
Calling memcpy on a null pointe Ron line 57 is a bad idea :)

[tresf]

@shlyakpavel thanks. Assuming you're comfortable with GitHub and C/C++, PRs are welcome. We are all just volunteers here and small fixes like this can be sent directly in the form of PRs.

[PhysSong]

Actually, there are several more lines like @shlyakpavel pointed out. Line 58 doesn't necessarily mean that atoms can be null though(it's valid to delete a null pointer, unlike free()). The actual part that initializes atoms is here:
https://github.com/LMMS/lmms/blob/7f0593c601a7f48fda193139f2d6bb3d76f10384/plugins/MidiImport/portsmf/allegro.h#L78-L81
Line 57 reduces to memcpy(new_atoms, NULL, 0);, which is a no-op in almost every implementation of memcpy. However, the C standard says the behavior is undefined.

Summary: the code works fine with most compilers, but the C standard doesn't guarantee it to work.

[softrabbit]

Actually, there are several more lines like @shlyakpavel pointed out. Line 58 doesn't necessarily mean that atoms can be null though(it's valid to delete a null pointer, unlike free()).

From my reading, it looks pretty much guaranteed that atoms is null the first time Alg_atoms::expand() is called, unless there's an allocation somewhere I missed.

[DomClark]

From my reading, it looks pretty much guaranteed that atoms is null the first time Alg_atoms::expand() is called, unless there's an allocation somewhere I missed.

It looks the same to me, but I think that's intended. The first time it's called, len and max_len are both zero, so it's just used to do the initial allocation once someone actually tries to put something in it. I don't think there's a logic error here, just the reliance upon undefined behaviour. If we're actually worried about this, then since the copy is a no-op when the source is null, it can just be moved inside the if test on the next line:

-    memcpy(new_atoms, atoms, len * sizeof(Alg_attribute));
-    if (atoms) delete[] atoms;
+    if(atoms) {
+        memcpy(new_atoms, atoms, len * sizeof(Alg_attribute));
+        delete[] atoms;
+    }