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

Jump to bottom

Function parsefmt cannot parse "I64d" when macro MP_HAVE_INT_EXTENSIONS is not defined (by default it is not defined)! #12944

Closed
nased0 opened this issue Feb 15, 2024 · 5 comments
Closed

Function parsefmt cannot parse "I64d" when macro MP_HAVE_INT_EXTENSIONS is not defined (by default it is not defined)! #12944

nased0 opened this issue Feb 15, 2024 · 5 comments
Labels
build

Comments

@nased0
Copy link

nased0 commented Feb 15, 2024 •
edited

I did this

I compiled libcurl statically with OpenSSL 1.1.1w (linked dynamically) by using CMake and Embarcadero bcc32 v. 7.0 compiler.
To do this I had to make some minor corrections in the code that are not related with the issue:

In file curl_setup.h I defined:

struct _stati64
{  
   unsigned int	st_dev;
   short	st_ino; 	/* Always zero ? */			     
   unsigned short	st_mode;	/* See above constants */		     
   short 	st_nlink;	/* Number of links. */			     
   short 	st_uid; 	/* User: Maybe significant on NT ? */	     
   short 	st_gid; 	/* Group: Ditto */			     
   unsigned int	st_rdev;	/* Seems useless (not even filled in) */     
  __int64	st_size;	/* File size in bytes */		     
  time_t	st_atime;	/* Access time (always 00:00 on FAT) */	     
  time_t	st_mtime;	/* Modified time */			     
  time_t	st_ctime;	/* Creation time */			     
};

In file vtls.c I added

#undef random
before function Curl_ssl_random.

in file openssl.c I added pragmas for linker:

#pragma comment(lib, "bcrypt.lib")
#pragma comment(lib, "Wldap32.lib")
#pragma comment(lib, "c:\Libs\common\libssle-1_1.lib")
#pragma comment(lib, "c:\Libs\common\libcryptoe-1_1.lib")

where openssl libs were converted to bcc32 OMF linker format by commands:

implib -a libcryptoe-1_1.lib libcrypto-1_1.dll 
implib -a libssle-1_1.lib libssl-1_1.dll

In files objects1.rsp (generated by CMake) I replaced UNIX path separators / by Windows path separators \

I expected the following

I expected my app to download a car report from historiapojazdu.gov.pl (polish Central Vehicle Register). But it crashed with code C0000005.
I managed to find the cause: function parsefmt defined in file mprintf.c cannot parse the substring "%I64d" when macro MP_HAVE_INT_EXTENSIONS is not defined. And by default, it is not defined!
So this function ignored "%I64d" and set the type FORMAT_STRING in va_input.type, then wrote value hex: 7FFFFFFF (cookie expiration date) to va_input.val.str by the code:

    switch(iptr->type) {
    case FORMAT_STRING:
      iptr->val.str = va_arg(arglist, char *);
      break;

And it caused a crash when accessing this invalid pointer.
The following parser code depends on macro MP_HAVE_INT_EXTENSIONS :
file mprintf.c
line 350

#if defined(MP_HAVE_INT_EXTENSIONS)
		case 'I':
		  if((fmt[0] == '3') && (fmt[1] == '2')) {
			flags |= FLAGS_LONG;
			fmt += 2;
		  }
		  else if((fmt[0] == '6') && (fmt[1] == '4')) {
			flags |= FLAGS_LONGLONG;
			fmt += 2;
		  }
		  else {
#if (SIZEOF_CURL_OFF_T > SIZEOF_LONG)
			flags |= FLAGS_LONGLONG;
#else
			flags |= FLAGS_LONG;
#endif
		  }
		  break;
#endif

curl/libcurl version

curl 8.6.0 (I built libcurl.lib only, no curl.exe because of linker error Unresolved external '_main' and because I don't need it).

operating system

Windows 10 Professional
@nased0
Copy link
Author

nased0 commented Feb 15, 2024 •
edited

This preprocessor code is no longer there (in file mprintf.c)!

#if (defined(__BORLANDC__) && (__BORLANDC__ >= 0x520))
(...)
#  define MP_HAVE_INT_EXTENSIONS
#endif

Instead it is now:

#if (defined(_WIN32_WCE)) || \
    (defined(__MINGW32__)) || \
    (defined(_MSC_VER) && (_MSC_VER >= 900) && (_INTEGRAL_MAX_BITS >= 64))
#  define MP_HAVE_INT_EXTENSIONS
#endif

But macro __BORLANDC__ is still used in the system.h file, to define macro CURL_FORMAT_CURL_OFF_T as "I64d", that is used by function get_netscape_format(const struct Cookie *co).

So when libcurl is compiled by any compiler that is not on this short list and uses macro CURL_FORMAT_CURL_OFF_T as "I64d", the code will crash when processing cookies!

IMO this function parsefmt should parse the format string "%I64d" unconditionally.
If this substring is in the function's format string, it is for a reason.
So this macro MP_HAVE_INT_EXTENSIONS should be removed from the code.

@jay
Copy link
Member

jay commented Feb 15, 2024

We probably don't support that compiler any longer.

/cc @vszakats

@jay jay added the build label Feb 15, 2024
@nased0
Copy link
Author

nased0 commented Feb 15, 2024

But it is still on the market as a part of Embarcadero C++ Builder or Embarcadero Rad Studio.
And some people are still using it to develop Windows apps.

So I propose to add it to this list of compilers in mprintf.c like this:

#if (defined(_WIN32_WCE)) || \
    (defined(__MINGW32__)) || \
    (defined(_MSC_VER) && (_MSC_VER >= 900) && (_INTEGRAL_MAX_BITS >= 64)) || \
    (defined(__BORLANDC__) && (__BORLANDC__ >= 0x520))
#  define MP_HAVE_INT_EXTENSIONS
#endif

to prevent this hard to find error.

@nased0
Copy link
Author

nased0 commented Feb 16, 2024

There are other compilers (defined in the file system.h) that will generate libcurl code affected by this parser error:

#elif defined(__POCC__)
#  if (__POCC__ < 280)
     (...)
#  elif defined(_MSC_VER)
#    define CURL_TYPEOF_CURL_OFF_T     __int64
#    define CURL_FORMAT_CURL_OFF_T     "I64d"

I am not sure that this macro MP_HAVE_INT_EXTENSIONS that limits the format string parser abilities (when not defined) is necessary. IMO it is a point of failure.

jay added a commit to jay/curl that referenced this issue Feb 16, 2024
@jay
mprintf: fix format prefix I32/I64 for Borland compilers
66fa4a1 
- Support I32 & I64 (eg: %I64d) for all Win32 builds.

Prior to this change mprintf support for the I format prefix, which is a
Microsoft extension, was dependent on the compiler used.

When Borland support was removed in fd7ef00 the prefix was then no
longer supported for that compiler; however since it's still possible to
build with Borland I'm restoring support for the prefix in this way.

Reported-by: Paweł Witas

Fixes curl#12944
Closes #xxxx
@jay jay mentioned this issue Feb 16, 2024
Closed
@jay
Copy link
Member

jay commented Feb 16, 2024

I am not sure that this macro MP_HAVE_INT_EXTENSIONS that limits the format string parser abilities (when not defined) is necessary. IMO it is a point of failure.

The format is Microsoft specific so I'm proposing broader support in #12950

@jay jay closed this as completed in e3a3bb3 Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

mprintf: fix format prefix I32/I64 for all windows compilers jay/curl
2 participants
@jay @nased0