Fix potential integer overflow at end of stream in media_foundation_helper

[wro-ableton]

If in SyncronousByteStream::Read, m_source.read() reaches the end of stream, boost::iostreams::file_descriptor returns -1 as an end of stream indicator. Previously, by casting the returned value to ULONG, an integer overflow would occur.
In the attached file, the observed behaviour then would be that the calling code in WMF would continue to attempt to read past the EOF indefinitely or until some unexpected state is reached.

To fix this, the end of stream indicator (-1) is caught and handled as "0 bytes read".

See the attached TestMp3.zip for a mp3 which triggers this edge case.

See Issue #73

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.10%. Comparing base (89eb1e3) to head (27b6639).

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #74   +/-   ##
=======================================
  Coverage   83.10%   83.10%           
=======================================
  Files          82       82           
  Lines        2024     2024           
=======================================
  Hits         1682     1682           
  Misses        342      342           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ni-mheppner]

Looks good to me.

[wro-ableton]

If you all are fine with the change, feel free to merge. We'd definitely appreciate a quick fix. 😁

[ni-mheppner]

Actually, are we sure that -1 is the only negative number a boost::iostreams::file_descriptor will ever return? According to the documentation it should be, but maybe checking the return code against negative numbers in general might be more robust.

[FalconPDX]

lgtm!

[FalconPDX]

would it make sense to define -1 as "endOfSequence" for readability purposes?

[ni-tsmit]

Probably better to avoid magic numbers indeed.

[wro-ableton]

I was wondering about that as well. It might be a bit defensive but maybe we could also catch any negative and translate the -1 end of sequence to 0, otherwise return either E_UNEXPECTED or E_FAIL. Any preference?

try
{
    const auto result =  m_source.read( reinterpret_cast<char*>( buffer ), toRead );
    if (result < -1) 
    {
        return E_FAIL;
    }
    *read = static_cast<ULONG>( result == -1 ? 0 : result );
    return S_OK;
}
catch ( const std::system_error& )
{
    return E_UNEXPECTED;
}
catch ( ... )
{
    return E_FAIL;
}

Alternatively, one could simply catch any "<0" cases with the following. S

*read = static_cast<ULONG>( result < 0 ? 0 : result );

[ni-tsmit]

Looks good to me too, if read() promises non-negative or -1 as return value then I think a check against a define with endOfSequence suffices and tells the clearest story.

[wro-ableton]

See fixup. So static constexpr auto endOfSequence =-1; but without the defensive return?

[ni-tsmit]

Probably better to avoid magic numbers indeed.