Opened 17 years ago
Closed 16 years ago
#92 closed Bug (Fixed)
DllStructGetData() truncation
Reported by: | Valik | Owned by: | Valik |
---|---|---|---|
Milestone: | 3.2.13.8 | Component: | AutoIt |
Version: | 3.2.10.0 | Severity: | None |
Keywords: | Cc: |
Description
DllStructGetData() truncates the last element of a (char/wchar) array to ensure null termination. The code needs modified to secretly allocate a larger buffer and secretly insert a null terminator outside the user-requested bounds so the user's data is not altered.
This is a display-only issue, the underlying data is not changed.
Attachments (1)
Change History (14)
Changed 17 years ago by Valik
comment:1 Changed 17 years ago by Jon
- Owner set to Jon
- Status changed from new to accepted
comment:2 in reply to: ↑ description ; follow-up: ↓ 3 Changed 17 years ago by Jpm
Replying to Valik:
DllStructGetData() truncates the last element of a (char/wchar) array to ensure null termination. The code needs modified to secretly allocate a larger buffer and secretly insert a null terminator outside the user-requested bounds so the user's data is not altered.
This is a display-only issue, the underlying data is not changed.
This will have drawback when the user use DllStructGetptr to access inner structure.
Scripter should respect the +1 if needed and only DllStructSetData will be protected against the overflow as DllCall using this struct can always overflow
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 17 years ago by Valik
Replying to Jpm:
This will have drawback when the user use DllStructGetptr to access inner structure.
Scripter should respect the +1 if needed and only DllStructSetData will be protected against the overflow as DllCall using this struct can always overflow
There is no drawback to allocating a larger buffer than requested. Once this ticket is closed and the thread where it was discovered fades away, user's won't even know that we allocate a larger buffer than they request. If your concern is a 1- or 2-byte buffer overflow won't be detected, then we can change the extra out-of-user-area bytes to read-only and trap any exception that occurs.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 17 years ago by Valik
... then we can change the extra out-of-user-area bytes to read-only and trap any exception that occurs.
Oh, and we should probably be doing this anyway. I suggested it once. It's a really easy way to catch buffer overflows with DllStruct stuff, so if we are going to allocate larger blocks for "house-keeping" data we should go ahead and mark it read-only and detect an attempts to write to it.
comment:5 in reply to: ↑ 4 Changed 17 years ago by Jpm
Replying to Valik:
... then we can change the extra out-of-user-area bytes to read-only and trap any exception that occurs.
Oh, and we should probably be doing this anyway. I suggested it once. It's a really easy way to catch buffer overflows with DllStruct stuff, so if we are going to allocate larger blocks for "house-keeping" data we should go ahead and mark it read-only and detect an attempts to write to it.
I don't understand how we can catch buffer overflow when dllstruct use by a DllCall that can set beyound whatever is hidden allocated
comment:6 Changed 16 years ago by Valik
- Severity set to None
Jon, is this fixed in your DllStruct implementation rewrite?
comment:7 Changed 16 years ago by Jon
No. I looked at it briefly, decided it was right as it was and then changed my mind, got confused and ignored it.
comment:8 Changed 16 years ago by Valik
Jon, the current behavior is wrong and causes data-loss of 1 character every time a string is read when strlen() == sizeof(buffer). For example, BSTR's don't have null terminators, they store the size in the first WORD (or something like that). Attempting to use this with an exact-sized BSTR-style string would cause data-loss of the last character.
AutoIt is assuming all array's are sized to correctly hold a null terminator and that it's okay to replace the last character with a null terminator when reading. This assumption is wrong since we don't know enough about how the data is being used to make that assumption.
comment:9 Changed 16 years ago by Valik
- Milestone set to 3.2.13.6
- Owner changed from Jon to Valik
- Resolution set to Fixed
- Status changed from accepted to closed
Fixed in version: 3.2.13.6
comment:10 Changed 16 years ago by Valik
- Milestone 3.2.13.6 deleted
- Resolution Fixed deleted
- Severity changed from None to Blocking
- Status changed from closed to reopened
Re-opening because I didn't fix this correctly the first time around. Also setting as blocking because I kind of made things worse.
comment:11 Changed 16 years ago by Valik
- Status changed from reopened to accepted
comment:12 Changed 16 years ago by Valik
- Severity changed from Blocking to None
comment:13 Changed 16 years ago by Valik
- Milestone set to 3.2.13.8
- Resolution set to Fixed
- Status changed from accepted to closed
Fixed in version: 3.2.13.8
Guidelines for posting comments:
- You cannot re-open a ticket but you may still leave a comment if you have additional information to add.
- In-depth discussions should take place on the forum.
For more information see the full version of the ticket guidelines here.
I'll sort this when I rewrite DllStruct.