Conversation
- Replace strcpy with strncpy in fe_warp_{affine,inverse_linear,piecewise_linear}.c
- Add bounds checking and ensure null termination for 256-byte buffers
- Add test_fe_warp_overflow to verify fix handles strings up to 511 chars
- Prevents potential memory corruption and crashes with long parameter strings
- Check malloc return value in WebRtcVad_Create to prevent NULL dereference - Add test_vad_alloc to verify memory allocation safety - Test includes 100 create/destroy cycles and NULL handling - Prevents segfault on memory allocation failure
- Replace repeated strcat() calls with single-pass memcpy() approach - Changes time complexity from O(n²) to O(n) - Add test_dict_strcat to verify correctness of optimization - Performance improvement: 1.24x-2.58x speedup (avg 1.70x) - CMU dictionary write time reduced from 304ms to 196ms (~35% faster)
lenzo-ka
changed the title
|
Oh dear! Thank you for this and sorry about the delay for reviewing (review to follow shortly) |
dhdaines
left a comment
dhdaines
left a comment
There was a problem hiding this comment.
Approved (the comment on CI is not critical)
.github/workflows/tests.yml
Outdated
| uses: actions/checkout@v4 | ||
| - name: Wait for dpkg lock | ||
| run: | | ||
| while sudo fuser /var/lib/dpkg/lock-frontend >/dev/null 2>&1; do |
There was a problem hiding this comment.
Is there a possibility that this could block forever? Perhaps it's better to fail fast...
| VadInst* WebRtcVad_Create(void) { | ||
| VadInstT* self = (VadInstT*)malloc(sizeof(VadInstT)); | ||
|
|
||
| if (self == NULL) { |
There was a problem hiding this comment.
Hmm. This should either be reported upstream (to Mozilla and/or Google) or we should update the WebRTC code from upstream... no longer sure where it lives exactly
| phones = ckd_calloc(1, phlen); | ||
| pos = phones; | ||
| for (j = 0; j < dict_pronlen(dict, i); ++j) { | ||
| strcat(phones, dict_ciphone_str(dict, i, j)); |
There was a problem hiding this comment.
Good catch. This is actually safe (because of the calculation of phlen) but it's really inefficient, and strcat should just be banished in any case...
| return; | ||
| } | ||
| is_neutral = NO; | ||
| strcpy(temp_param_str, param_str); |
There was a problem hiding this comment.
hoo boy ... yes this is some CMU legacy code ...
2 participants
Fix Buffer and Performance Issues
This addresses buffer issues and a performance tweak on reading dicts
Summary of Changes
1. Buffer Overflow Protection (CVE-worthy)
strcpy()usage in feature warping modules that could cause crashes with long parameter stringsfe_warp_affine.c,fe_warp_inverse_linear.c,fe_warp_piecewise_linear.cstrncpy()with proper bounds checking and null termination2. NULL Pointer Dereference Prevention
malloc()in WebRTC VAD modulewebrtc_vad.c3. Dictionary Write Performance Optimization
dict.cstrcat()with single-passmemcpy()Testing
All fixes include comprehensive unit tests:
test_fe_warp_overflow- Validates buffer overflow protectiontest_vad_alloc- Tests memory allocation safetytest_dict_strcat- Verifies dictionary output correctnessAll 95 tests pass. No breaking changes.
Impact