Skip to content

Fixed warning about overflow, rtl-string line 3663. #4

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fixed warning about overflow, rtl-string line 3663. #4

wants to merge 1 commit into from

Conversation

mudhairless
Copy link
Contributor

ASC function return value is uinteger, uinteger - uinteger = uinteger.
Previous code would put incorrect value into chardiff.

@mudhairless
Fixed warning about overflow, rtl-string line 3663.
e6541c5 
ASC function return value is uinteger, uinteger - uinteger = uinteger.
Previous code would put incorrect value into chardiff.
@dkl
Copy link
Member

dkl commented Jan 16, 2014

Hello,

I only get the warning when building with 0.24, but not with 0.90.1. There have been some changes then where we only warn when loosing information, but not when it's just a signed/unsigned conversion. asc( "A" ) - asc( "a" ) returns 4294967264 (UInteger), but when assigned to chardiff (Integer) that still becomes -32, same as cint( asc( "A" ) ) - cint( asc( "a" ) ).

Either way though I agree, it's probably still better to use the casts to Integer there.

dkl added a commit that referenced this pull request Jan 16, 2014
@dkl
Add cint() to avoid warnings with fbc 0.24
c21f6fc 
as suggested at <#4>.
@dkl
Copy link
Member

dkl commented Jan 16, 2014

Ok, I've made the change in c21f6fc.

@dkl dkl closed this Jan 16, 2014
@jklwn jklwn mentioned this pull request Sep 7, 2019
Closed
dkl added a commit to dkl/fbc that referenced this pull request Jul 18, 2022
@dkl
tests: Fix stack corruption due to CLEAR in MID() unit tests
10596fa 
CLEAR takes the "object" to clear by reference, not an address by value.
The extra @ (address-of) lead to clearing a temp var on stack (and
likely overflowing it) instead of clearing the wanted variable,
i.e. undefined behaviour.

Reported at: freebasic#382
gcc AddressSanitizer finds this issue aswell:
==279298==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffb16875e8 at pc 0x7fb7f4a01c23 bp 0x7fffb16874a0 sp 0x7fffb1686c48
WRITE of size 200 at 0x7fffb16875e8 thread T0
    #0 0x7fb7f4a01c22 in __interceptor_memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:799
    freebasic#1 0xa3fb11 in TESTS::FBC_TESTS::UDT_WSTRING_::MIDSTMT::ASCII() (/home/daniel/fb/fbc/tests/fbc-tests+0xa3fb11)
    freebasic#2 0xcc536c in FBCU::RUN_TESTS(bool, bool) src/fbcunit.bas:649
    freebasic#3 0xcc134e in main (/home/daniel/fb/fbc/tests/fbc-tests+0xcc134e)
    freebasic#4 0x7fb7f4506d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    freebasic#5 0x7fb7f4506e3f in __libc_start_main_impl ../csu/libc-start.c:392
    freebasic#6 0x413364 in _start (/home/daniel/fb/fbc/tests/fbc-tests+0x413364)

Fixes: 135f364 "udt-wstring: MID statement will accept UDT as Z|WSTRING"
dkl added a commit to dkl/fbc that referenced this pull request Jul 18, 2022
@dkl
tests: Fix stack buffer overflow due to indexing in MID() tests
8c25ce4 
Given
	dim e1 as wstring * BUFFERSIZE
	dim n1 as integer = BUFFERSIZE
then e1[n1] is an out-of-bounds access. I'm not entirely sure if the
change is sane with regards to the intent of the code (maybe e1 is
supposed to be BUFFERSIZE + 1?), but least it's not overflowing anymore.

Found with gcc AddressSanitizer:
[...]
==325124==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd6b7ecd28 at pc 0x000000b97cfb bp 0x7ffd6b7ec800 sp 0x7ffd6b7ec7f0
WRITE of size 4 at 0x7ffd6b7ecd28 thread T0
    #0 0xb97cfa in TESTS::FBC_TESTS::WSTRING_::MIDSTMT::ASCII() wstring/midstmt.bas:86
    freebasic#1 0xc56259 in FBCU::RUN_TESTS(bool, bool) src/fbcunit.bas:649
    freebasic#2 0xc52533 in main (/home/daniel/fb/fbc/tests/fbc-tests+0xc52533)
    freebasic#3 0x7fe4d3ddad8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    freebasic#4 0x7fe4d3ddae3f in __libc_start_main_impl ../csu/libc-start.c:392
    freebasic#5 0x4082e4 in _start (/home/daniel/fb/fbc/tests/fbc-tests+0x4082e4)

Address 0x7ffd6b7ecd28 is located in stack of thread T0 at offset 1000 in frame
    #0 0xb97420 in TESTS::FBC_TESTS::WSTRING_::MIDSTMT::ASCII() wstring/midstmt.bas:70

  This frame has 8 object(s):
    [32, 100) 'DST$1'
    [144, 212) 'SRC$1'
    [256, 456) 'W1$8'
    [528, 728) 'W2$8'
    [800, 1000) 'E1$8' <== Memory access at offset 1000 overflows this variable
    [1072, 1272) 'W1$10'
    [1344, 1544) 'W2$10'
    [1616, 1816) 'E1$10'
[...]

Fixes: 135f364 "udt-wstring: MID statement will accept UDT as Z|WSTRING"
dkl added a commit to dkl/fbc that referenced this pull request Jul 18, 2022
@dkl
tests: Fix stack corruption due to CLEAR in MID() unit tests
ddaf772 
CLEAR takes the "object" to clear by reference, not an address by value.
The extra @ (address-of) lead to clearing a temp var on stack (and
likely overflowing it) instead of clearing the wanted variable,
i.e. undefined behaviour.

Reported at: freebasic#382
gcc AddressSanitizer finds this issue aswell:
```
==279298==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffb16875e8 at pc 0x7fb7f4a01c23 bp 0x7fffb16874a0 sp 0x7fffb1686c48
WRITE of size 200 at 0x7fffb16875e8 thread T0
    #0 0x7fb7f4a01c22 in __interceptor_memset ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:799
    freebasic#1 0xa3fb11 in TESTS::FBC_TESTS::UDT_WSTRING_::MIDSTMT::ASCII() (/home/daniel/fb/fbc/tests/fbc-tests+0xa3fb11)
    freebasic#2 0xcc536c in FBCU::RUN_TESTS(bool, bool) src/fbcunit.bas:649
    freebasic#3 0xcc134e in main (/home/daniel/fb/fbc/tests/fbc-tests+0xcc134e)
    freebasic#4 0x7fb7f4506d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    freebasic#5 0x7fb7f4506e3f in __libc_start_main_impl ../csu/libc-start.c:392
    freebasic#6 0x413364 in _start (/home/daniel/fb/fbc/tests/fbc-tests+0x413364)
```

Fixes: 135f364 "udt-wstring: MID statement will accept UDT as Z|WSTRING"
dkl added a commit to dkl/fbc that referenced this pull request Jul 18, 2022
@dkl
tests: Fix stack buffer overflow due to indexing in MID() tests
f38d0d8 
Given
	dim e1 as wstring * BUFFERSIZE
	dim n1 as integer = BUFFERSIZE
then e1[n1] is an out-of-bounds access. I'm not entirely sure if the
change is sane with regards to the intent of the code (maybe e1 is
supposed to be BUFFERSIZE + 1?), but least it's not overflowing anymore.

Found with gcc AddressSanitizer:
```
==325124==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd6b7ecd28 at pc 0x000000b97cfb bp 0x7ffd6b7ec800 sp 0x7ffd6b7ec7f0
WRITE of size 4 at 0x7ffd6b7ecd28 thread T0
    #0 0xb97cfa in TESTS::FBC_TESTS::WSTRING_::MIDSTMT::ASCII() wstring/midstmt.bas:86
    freebasic#1 0xc56259 in FBCU::RUN_TESTS(bool, bool) src/fbcunit.bas:649
    freebasic#2 0xc52533 in main (/home/daniel/fb/fbc/tests/fbc-tests+0xc52533)
    freebasic#3 0x7fe4d3ddad8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    freebasic#4 0x7fe4d3ddae3f in __libc_start_main_impl ../csu/libc-start.c:392
    freebasic#5 0x4082e4 in _start (/home/daniel/fb/fbc/tests/fbc-tests+0x4082e4)

Address 0x7ffd6b7ecd28 is located in stack of thread T0 at offset 1000 in frame
    #0 0xb97420 in TESTS::FBC_TESTS::WSTRING_::MIDSTMT::ASCII() wstring/midstmt.bas:70

  This frame has 8 object(s):
    [32, 100) 'DST$1'
    [144, 212) 'SRC$1'
    [256, 456) 'W1$8'
    [528, 728) 'W2$8'
    [800, 1000) 'E1$8' <== Memory access at offset 1000 overflows this variable
    [1072, 1272) 'W1$10'
    [1344, 1544) 'W2$10'
    [1616, 1816) 'E1$10'
[...]
```

Fixes: 135f364 "udt-wstring: MID statement will accept UDT as Z|WSTRING"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mudhairless @dkl