reads from uninitialized memory

sezero / mikmod

Mikmod Sound System (mirror of git repo at https://sf.net/projects/mikmod/)

http://mikmod.sourceforge.net/

69 stars 21 forks source link

reads from uninitialized memory #67

Closed sezero closed 2 years ago

sezero commented 2 years ago

Changing MikMod_malloc to use plain malloc and not calloc revealed actual defects, i.e. unitialized memory reads here and there.

So far, I pushed the following commits to fix the ones I encountered:

explicitly use MikMod_calloc for SAMPLOAD and MikMod_amalloc (https://github.com/sezero/mikmod/commit/517966a2ab4f52d3403a3cd8c18ae7eda3c15bbe)
explicitly use MikMod_calloc in ReadLinedComment for FAR (https://github.com/sezero/mikmod/commit/4693928ed823418015356eff794c2951c1543570)
ASY_Load: avoid uninitialized reads from DupStr (https://github.com/sezero/mikmod/commit/605e9f3474efcacb7846196bc5206b65a08593dd)
MikMod_InfoDriver: ensure proper nul termination of list (https://github.com/sezero/mikmod/commit/9db6d2f0c74be46e1ee3a9a44a79af5bfc2f69b7)
mikmod player, mconfedit.c: make skip_number saner (https://github.com/sezero/mikmod/commit/2a1b1f6659b77f90da19e158a6c22319d6712b2a)

More may follow.

@AliceLR: Can you have a look at the last three commits to make sure they are correct?

AliceLR commented 2 years ago

ASY_Load: avoid uninitialized reads from DupStr (605e9f3)

This looks OK to me. The ASYLUM format doesn't have a module name field that I've ever seen, and I don't know why this loader has a struct member for it. I don't think I've seen 22 char sample names in them but IIRC they're valid in MODs and it's likely MOD2AMF outputs whatever sample names it was given.

MikMod_InfoDriver: ensure proper nul termination of list (9db6d2f)

I can't tell if this one was actually a problem or not (I think it would have output ## \0 for the final entry?), but the new handling is cleaner. This seems OK.

mikmod player, mconfedit.c: make skip_number saner (2a1b1f6)

This looks OK to me. The *str conditions might be redundant, but they don't hurt. (Also, LOL at the signed-to-signed casts for isdigit.)

sezero commented 2 years ago

Thanks for the reviews

MikMod_InfoDriver: ensure proper nul termination of list (9db6d2f)

I can't tell if this one was actually a problem or not (I think it would have output ## \0 for the final entry?), but the new handling is cleaner. This seems OK.

This showed itself in mikmod player when you hit c where it plays with the driver list in evil ways, and skip_number used to hit an uninitialized byte.

sezero commented 2 years ago

The *str conditions might be redundant, but they don't hurt.

Well yeah, I was blind I guess. Removed them.

sezero commented 2 years ago

Changed MikMod_malloc to use plain malloc and not calloc, as of commit 544deb0. The defects revealed by this, i.e. several unitialized memory reads here and there, has been fixed so far. Let's be on the watch for any remaining issues - long live valgrind...

Closing this ticket.