Skip to content
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

mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

Closed
wants to merge 8 commits into from
Closed

mbed TLS 1.3: Fix compilation warnings with VS2015 x64 #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
83081c5
Fix MSVC warning in sample programs
Jul 6, 2017
2a0344f
Fix MSVC warning in net.c
Jul 6, 2017
c4b659c
Fix type in net.c comment
Jul 6, 2017
e8c587e
Add brackets around function macro arguments
Jul 6, 2017
5ad6be7
Add brackets around net.c macro arguments
Jul 12, 2017
28d7d3a
Change param type for x509_parse_int() to fix warn
Jul 31, 2017
e4fd1ba
Fix coding style in x509_parse_int()
Jul 31, 2017
eb01d3c
Add ChangeLog entry
Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ChangeLog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
mbed TLS ChangeLog (Sorted per branch, date)

= mbed TLS 1.3.x released xxxx-xx-xx

Bugfix
* Fix Visual Studio implicit cast compilation warnings in the net.c and
x509.c modules and some sample applications.

= mbed TLS 1.3.20 released 2017-06-21

Security
Expand Down
16 changes: 12 additions & 4 deletions library/net.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@
#endif
#endif /* _MSC_VER */

#define read(fd,buf,len) recv(fd,(char*)buf,(int) len,0)
#define write(fd,buf,len) send(fd,(char*)buf,(int) len,0)
#define read(fd,buf,len) recv( fd, (char*)( buf ), (int)( len ), 0 )
#define write(fd,buf,len) send( fd, (char*)( buf ), (int)( len ), 0 )
#define close(fd) closesocket(fd)

static int wsa_init_done = 0;
Expand Down Expand Up @@ -92,6 +92,14 @@ static int wsa_init_done = 0;

#endif /* ( _WIN32 || _WIN32_WCE ) && !EFIX64 && !EFI32 */

/* Some MS functions want int and MSVC warns if we pass size_t,
* but the standard functions use socklen_t, so cast only for MSVC */
#if defined(_MSC_VER)
#define MSVC_INT_CAST (int)
#else
#define MSVC_INT_CAST
#endif

#include <stdlib.h>
#include <stdio.h>

Expand Down Expand Up @@ -202,7 +210,7 @@ int net_connect( int *fd, const char *host, int port )
continue;
}

if( connect( *fd, cur->ai_addr, cur->ai_addrlen ) == 0 )
if( connect( *fd, cur->ai_addr, MSVC_INT_CAST cur->ai_addrlen ) == 0 )
{
ret = 0;
break;
Expand Down Expand Up @@ -299,7 +307,7 @@ int net_bind( int *fd, const char *bind_ip, int port )
continue;
}

if( bind( *fd, cur->ai_addr, cur->ai_addrlen ) != 0 )
if( bind( *fd, cur->ai_addr, MSVC_INT_CAST cur->ai_addrlen ) != 0 )
{
close( *fd );
ret = POLARSSL_ERR_NET_BIND_FAILED;
Expand Down
16 changes: 11 additions & 5 deletions library/x509.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,14 +480,20 @@ int x509_get_name( unsigned char **p, const unsigned char *end,
}
}

static int x509_parse_int(unsigned char **p, unsigned n, int *res){
static int x509_parse_int( unsigned char **p, size_t n, int *res )
{
*res = 0;
for( ; n > 0; --n ){
if( ( **p < '0') || ( **p > '9' ) ) return POLARSSL_ERR_X509_INVALID_DATE;

for( ; n > 0; --n )
{
if( ( **p < '0') || ( **p > '9' ) )
return( POLARSSL_ERR_X509_INVALID_DATE );

*res *= 10;
*res += (*(*p)++ - '0');
*res += ( *(*p)++ - '0' );
}
return 0;

return( 0 );
}

static int x509_date_is_valid(const x509_time *time)
Expand Down
2 changes: 1 addition & 1 deletion programs/ssl/ssl_client2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1267,7 +1267,7 @@ int main( int argc, char *argv[] )

len = polarssl_snprintf( (char *) buf, sizeof(buf) - 1, GET_REQUEST,
opt.request_page );
tail_len = strlen( GET_REQUEST_END );
tail_len = (int) strlen( GET_REQUEST_END );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should consider changing tail_len to size_t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the code again and I think that if we change this to size_t we will eventually need to use casts back to int. The problem is that functions such as snprintf() return int. Also, note that all the input parameters, etc are of type int so its not that straight forward to change one type. I think its best if we just investigate these problems as part of #1007


/* Add padding to GET request to reach opt.request_size in length */
if( opt.request_size != DFL_REQUEST_SIZE &&
Expand Down
6 changes: 4 additions & 2 deletions programs/ssl/ssl_mail_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@
#include <unistd.h>
#else
#include <io.h>
#define read _read
#define write _write
#define read(fd, buf, len) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally, I think the MACRO checks should be different, to make it more readable(although this is not part of this PR):

#if defined (_MSC_VER)
#else
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I think I will skip this change for this PR.

_read( fd, (void *)( buf ), (unsigned int)( len ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, we could lose data here. parameter len is size_t, which is 64 bit in x64. if for some reason, len would be MAX_UINT, we will downcast to unsigned int and lose information. This casting only removes the warning(symptom), not the data loss.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could. In fact, I copied this code from the development branch: https://github.com/ARMmbed/mbedtls/blob/development/library/net_sockets.c#L67. I do not think this is a major problem as this is only sample code (even the net_sockets.c), but perhaps we could improve the quality of this code. Do you think I should raise an issue on this or fix it here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should raise an issue for this, but if possible, fis it here while we are modifying this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem in this case is that in Unix we use the function read() while in Windows we use the function _read. Annoyingly the signature for these two functions is different, hence why we use the cast in the Window case as the type of len in linux is size_t. We could try to add some checks to make sure that the input parameter is not overflowed as you say, but we would need to use #if guards and I think that this just complicates the sample code...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least comment this, so people using this sample code would be aware of this risk

#define write(fd, buf, len) \
_write( fd, (const void *)( buf ), (unsigned int)( len ) )
#endif

#if defined(_WIN32) || defined(_WIN32_WCE)
Expand Down
2 changes: 1 addition & 1 deletion programs/ssl/ssl_server2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1791,7 +1791,7 @@ int main( int argc, char *argv[] )
unsigned char *larger_buf;

ori_len = ret;
extra_len = ssl_get_bytes_avail( &ssl );
extra_len = (int) ssl_get_bytes_avail( &ssl );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to change extra_len to size_t type, and thus remove the need for casting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good idea, however everything I have done here is backport the fix from the development branch. Do you think I should try to fix the problem here or raise another issue to address this in all the 3 branches?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both:)
I think you should fix here, as we are already modifying the file, and raise another issuer for the other two branches

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RonEld: Even if we change this to size_t we will eventually need to cast as the return type of ssl_read() is int. In any case, we will not overflow the buffer because according to the RFC 5246 the length of the TLS record's content cannot exceed 2^14.


larger_buf = polarssl_malloc( ori_len + extra_len + 1 );
if( larger_buf == NULL )
Expand Down