Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling unit tests for MSVC compiler + fix MSVC compiler and static analyzer warnings #5

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

alexey-malov
Copy link

@alexey-malov alexey-malov commented Nov 22, 2023

*Issue #4
*Issue #6

Description of changes:

  • Fixed unit tests linking under MSVC
  • Made casts of size_t to int in C-code
  • Fixed possible null pointer dereferences reported by MSVC static analyzer
  • Fixed various MSVC Level 4 warnings
  • Fix webvtt comment parsing (Error parsing WEBVTT tag with a comment #6)

I checked compilation and ran tests under MSVC 2015, 2017, 2019, 2022 and gcc 9.4.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

This fixes MSVC compiler warning
Variable `status` in a cycle shadows the `status` variable from the outer scope.
I can omit shadowing by reusing the variable from the outer scope since it is not used after the cycle.
The `node` argument must be checked against NULL before it is dereferenced.
- Fix possible null pointer dereference in webvtt_proc_cueline function.
  In cue argument must be checked against NULL
  before it is dereferenced on lines 520 and 541.
  Testing cue pointer for NULL on line 525 is too late.
- Fix `token` variable shadowin on line 608
- Fix possible problems in webvtt_read_cuetext function.
  - In line 844 the assertion expression must check the value of self->top->type
    instead of assigning it.
  - Line 866: It is necessary check self->line_buffer.d for NULL because
    it can be NULL due to check on line 852. Otherwise it is possible
    to dereference a NULL pointer on line 889.
- Fix false positive code analyzer warning on possible intermediate result truncation on line 1067:
    ( ch - '0' ) produces 32-bit value which is then converted to 64 bit when added to `result * 10`.
Add explicit type casts when size_t is converted to int

On line 345 we must check that `str->d` is not NULL because
MSVC code analyzer can't prove that `str->d` is not NULL when
webvtt_create_string succeeds.
`str->d` is assigned to `d` on line 348 and then `d`
is dereferenced on line 364.
Checking `str->d` against NULL helps the analyzer to understand that
there is no NULL pointer derefernce on line 364.
- Calling fstream::close is not noexcept. Wrapping it in try-catch block
  helps static code analyzer to understand that destructor will not throw.
- Replaced c-style cast with static_cast.
- C++ core guidelines recommend to initialize local variables.
  Moving reading cycle and buffer into immediately invoked lambda
  expression reduces number of variables in outer scope and
  allows to initialize `status` and `finishStatus`.
C++ code should use C++-style casts or list initialization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant