[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15545822#comment-15545822
]
Jens Geyer commented on THRIFT-3941:
You know that WinXP is no longer supported (except for POS systems)?
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15545841#comment-15545841
]
Ted Wang commented on THRIFT-3941:
--
I know, but any client code can still set Windows XP to be the minimum platform
SDK through TARGET_WIN_XP, which will mean that they get the "XP
implementation". As a result, you can still get into this situation running on
Windows 7/10 unless we remove the XP implementation completely. The fix is also
pretty straight-forward though.
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15545886#comment-15545886
]
Ted Wang commented on THRIFT-3941:
--
In fact, Thrift specifically targets Windows XP for broadest compatibility, so
by default, all clients get the Win XP implementation with this undefined
behavior.
This is in the README:
## Windows version compatibility
The Thrift library targets Windows XP for broadest compatbility. A notable
difference is in the Windows-specific implementation of the socket poll
function. To target Vista, Win7 or other versions, comment out the line
#define TARGET_WIN_XP.
And here is the source:
https://github.com/apache/thrift/blob/master/lib/cpp/src/thrift/windows/config.h#L44
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15545909#comment-15545909
]
ASF GitHub Bot commented on THRIFT-3941:
GitHub user tpcwang opened a pull request:
https://github.com/apache/thrift/pull/1107
THRIFT-3941 WinXP version of thrift_poll() relies on undefined behavior by
passing a destructed variable to select()
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tpcwang/thrift THRIFT-3941
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1107.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1107
commit d1c0d331992014f36b221ea707943cbaa3bfb3a3
Author: tpcwang
Date: 2016-10-04T16:34:37Z
Fix WinXP version of thrift_poll to not use destructed time_out
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15545972#comment-15545972
]
ASF GitHub Bot commented on THRIFT-3941:
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1107
Looks good to me. :+1:
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15548328#comment-15548328
]
ASF GitHub Bot commented on THRIFT-3941:
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1107
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
> Fix For: 0.10.0
>
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (THRIFT-3941) WinXP version of thrift_poll() relies on undefined behavior by passing a destructed variable to select()
[
https://issues.apache.org/jira/browse/THRIFT-3941?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15548588#comment-15548588
]
James E. King, III commented on THRIFT-3941:
This would be an issue that [Coverity
Scan|https://scan.coverity.com/projects/thrift] should have picked up, and
perhaps it did, but we're not taking action on the results?
> WinXP version of thrift_poll() relies on undefined behavior by passing a
> destructed variable to select()
>
>
> Key: THRIFT-3941
> URL: https://issues.apache.org/jira/browse/THRIFT-3941
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Library
>Affects Versions: 0.9.3
>Reporter: Ted Wang
>Assignee: Ted Wang
> Fix For: 0.10.0
>
>
> thrift_poll() for WINVER <= 0x0502 in thrift/windows/WinFnctl.cpp shadows the
> 'time_out' variable, and it ends up passing the destructed copy to select():
> timeval time_out;
> timeval* time_out_ptr = NULL;
> if (timeout >= 0) {
> timeval time_out = {timeout / 1000, (timeout % 1000) * 1000};
> time_out_ptr = &time_out;
> } else { // to avoid compiler warnings
> (void)time_out;
> (void)timeout;
> }
> int sktready = select(1, read_fds_ptr, write_fds_ptr, NULL, time_out_ptr);
> Stepping through this code in the debugger, it looks like MSVC reserves a
> large enough stack frame to avoid overwriting the variable when calling
> select(), which may be why this hasn't been caught yet.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
