Modify

Opened 11 years ago

Closed 11 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)

DllStructTrunc.au3 (263 bytes) - added by Valik 11 years ago.

Download all attachments as: .zip

Change History (14)

Changed 11 years ago by Valik

comment:1 Changed 11 years ago by Jon

  • Owner set to Jon
  • Status changed from new to accepted

I'll sort this when I rewrite DllStruct.

comment:2 in reply to: ↑ description ; follow-up: Changed 11 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: Changed 11 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: Changed 11 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 11 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 11 years ago by Valik

  • Severity set to None

Jon, is this fixed in your DllStruct implementation rewrite?

comment:7 Changed 11 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 11 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 11 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 11 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 11 years ago by Valik

  • Status changed from reopened to accepted

comment:12 Changed 11 years ago by Valik

  • Severity changed from Blocking to None

comment:13 Changed 11 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.

Add Comment

Modify Ticket

Action
as closed The owner will remain Valik.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.