jstrait / wavefile

A Ruby gem for reading and writing sound files in Wave format (*.wav)
https://wavefilegem.com
MIT License
208 stars 24 forks source link

Misleading error message if "fmt " chunk extension is too large to fit into chunk #39

Closed jstrait closed 1 year ago

jstrait commented 2 years ago

As an example, suppose a "fmt " chunk has a stated size of 30 bytes, and the format code is not 1 (and therefore the chunk should have an extension). Since a "fmt " chunk extension body always starts at byte 18 (0-based), this means there are 12 bytes available for the chunk extension. If the extension has a reported size larger than 12 bytes, then the chunk extension will overflow out of the chunk.

In v1.1.1, this scenario will always cause InvalidFormatError to be raised. However, that happens by an accident of the implementation, and the error message will be misleading: "Not a supported wave file. The format chunk extension is shorter than expected." Although the extension could be shorter than expected, that's not guaranteed, and won't really be the source of the error.

What should happen in this scenario? If some of the bytes that overflow are part of required fields for the chunk extension (such as say, the first 22 bytes of a WAVE_FORMAT_EXTENSIBLE chunk extension), then it seems clear this should result in an error. If the overflowed bytes are just extra inert bytes, then you could argue that no error needs to be raised. However, that could lead to inconsistent behavior between different format codes, because the gem would have to understand the chunk extension format of every one of the many possible format codes in order to implement this in a consistent way. Also, always raising an error is how previous versions of the gem behave. Therefore, it seems preferable to always raise an error if the chunk extension overflows.

This is closely related to #33, because that bug and this one are caused by the same line of code. Despite the overlap, this bug is being opened since the two issues seem distinct from a behavior perspective.

jstrait commented 1 year ago

This is now fixed in v1.1.2, so closing this issue.