sprintf warnings

[Mark Rotteveel]

In the macOS build, I noticed warnings about sprintf:

```
src/cloop/Lexer.cpp:157:5: warning: 'sprintf' is deprecated: This
function is provided for compatibility reasons only. Due to security
concerns inherent in the design of sprintf(3), it is highly recommended
that you use snprintf(3) instead. [-Wdeprecated-declarations]
sprintf(buffer, "%s:%i:%i: error:
Invalid hexadecimal prefix.",
^
```

Any objections if I try and replace those with snprintf?

Mark
--
Mark Rotteveel

[Alex Peshkoff]

This makes no sense from security POV - mentioned code is used only at build time. But if you wish to make it look better - please :-)

[Mark Rotteveel]

It makes sense from a "eliminate warnings as much as possible" POV.
Current builds are drowning in all kinds of warnings on all platforms.
That makes warnings rather useless.

Also, this was just an example, sprintf warnings are also generated by
other code than cloop.

In any case, I'll prepare a PR for this.

Mark
--
Mark Rotteveel

[Mark Rotteveel]

I see a macro SNPRINTF defined as a compatibility shim, while - as far
as I know - all compilers we use support it. Additionally, there is
fb_utils::snprintf to assert the returned length.

Should I call fb_utils::snprintf instead of bare snprintf, or should I
use SNPRINTF?

I also notice that this isn't done consistently right now (sometimes
snprintf is called directly, instead of using fb_utils::snprintf or
SNPRINTF). Should that be addressed as well?

Mark
--
Mark Rotteveel

[Alex Peshkoff]

On 4/22/25 10:23, 'Mark Rotteveel' via firebird-devel wrote:

It makes sense from a "eliminate warnings as much as possible" POV. Current builds are drowning in all kinds of warnings on all platforms.

Not true. On linux I get just a few warnings using gcc13. Most of them are signed/unsigned comparison - and yes, that's not good, may cause real troubles.

[Alex Peshkoff]

On 4/22/25 10:56, 'Mark Rotteveel' via firebird-devel wrote:

On 19/04/2025 08:54, 'Mark Rotteveel' via firebird-devel wrote:

In the macOS build, I noticed warnings about sprintf:

```
  src/cloop/Lexer.cpp:157:5: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]
                                 sprintf(buffer, "%s:%i:%i: error: Invalid hexadecimal prefix.",
                                 ^
```

Any objections if I try and replace those with snprintf?

I see a macro SNPRINTF defined as a compatibility shim, while - as far as I know - all compilers we use support it.

Long ago snprintf behaved in different manner on different platforms - it was return code related (do not remember details - something like -1 or -required_size on overflow).  SNPRINTF was targeted to address this.
May be it was solved since that days - did not pay much attention.

Additionally, there is fb_utils::snprintf to assert the returned length.

Should I call fb_utils::snprintf instead of bare snprintf, or should I use SNPRINTF?

Do not know. BTW, SNPRINTF & fb_utils::snprintf are not accessible in all places, for example in cloop one can't use them.

I also notice that this isn't done consistently right now (sometimes snprintf is called directly, instead of using fb_utils::snprintf or SNPRINTF). Should that be addressed as well?

I'm not sure we have single way to solve that everywhere. For example I remember that in fb_string I've added a code that takes care about different (per platform) return values itself. Doubt that replacing it with some generic macro/function will make it work better.

[Mark Rotteveel]

On 22/04/2025 12:51, Alex Peshkoff wrote:
> On 4/22/25 10:56, 'Mark Rotteveel' via firebird-devel wrote:
>> On 19/04/2025 08:54, 'Mark Rotteveel' via firebird-devel wrote:
>>> In the macOS build, I noticed warnings about sprintf:
>>>
>>> ```
>>>   src/cloop/Lexer.cpp:157:5: warning: 'sprintf' is deprecated: This
>>> function is provided for compatibility reasons only.  Due to security
>>> concerns inherent in the design of sprintf(3), it is highly
>>> recommended that you use snprintf(3) instead. [-Wdeprecated-
>>> declarations]
>>>                                  sprintf(buffer, "%s:%i:%i: error:
>>> Invalid hexadecimal prefix.",
>>>                                  ^
>>> ```
>>>
>>> Any objections if I try and replace those with snprintf?
>>
>> I see a macro SNPRINTF defined as a compatibility shim, while - as far
>> as I know - all compilers we use support it.
>
> Long ago snprintf behaved in different manner on different platforms -
> it was return code related (do not remember details - something like -1
> or -required_size on overflow).  SNPRINTF was targeted to address this.
> May be it was solved since that days - did not pay much attention.
>
>> Additionally, there is fb_utils::snprintf to assert the returned length.
>>
>> Should I call fb_utils::snprintf instead of bare snprintf, or should I
>> use SNPRINTF?
>>
>
> Do not know. BTW, SNPRINTF & fb_utils::snprintf are not accessible in
> all places, for example in cloop one can't use them.

OK, but should I use fb_utils::snprintf if it is available, and
otherwise bare snprintf? Or should I use, in order of availability,
fb_utils::snprintf, otherwise SNPRINTF, or otherwise bare snprintf?

>> I also notice that this isn't done consistently right now (sometimes
>> snprintf is called directly, instead of using fb_utils::snprintf or
>> SNPRINTF). Should that be addressed as well?
>
> I'm not sure we have single way to solve that everywhere. For example I
> remember that in fb_string I've added a code that takes care about
> different (per platform) return values itself. Doubt that replacing it
> with some generic macro/function will make it work better.

The return value should be the same on each platform, assuming C99 or
newer compliance, though Microsoft's _snprintf (not to be confused with
snprintf) and *old* versions of snprintf (MSVC/MSVC++ before 14.0
(2015)) do not correctly account for null-termination:

"""
Beginning with the UCRT in Visual Studio 2015 and Windows 10, snprintf
is no longer identical to _snprintf. The snprintf behavior is now C99
standard conformant. The difference is that if you run out of buffer,
snprintf null-terminates the end of the buffer and returns the number of
characters that would have been required whereas _snprintf doesn't
null-terminate the buffer and returns -1.
"""
(from
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170)

Mark
--
Mark Rotteveel

[Dimitry Sibiryakov]

Alex Peshkoff wrote 22.04.2025 12:49:
> On linux I get just a few warnings using gcc13. Most of them are signed/unsigned
> comparison - and yes, that's not good, may cause real troubles.

In CI logs I see an enormous number of warnings about members' override.

--
WBR, SD.

[Mark Rotteveel]

This will probably depends on which warnings you have enabled.

The Linux build on GitHub Actions, for example, also complains about a
lot of -Winconsistent-missing-override,
-Wdeprecated-copy-with-user-provided-copy, -Wvarargs, -Wparentheses,
-Wswitch, -Wdangling-else and others

Mark
--
Mark Rotteveel

[Adriano dos Santos Fernandes]

Which makes no sense to fix before people review and merge large work
like schemas. Also worth wait for metacache finish.


Adriano

[Jim Starkey]

I subscribe to the school of thought that believes that all code should
be compiled warning-free and that this should be enforce by the compiler
switches that promote warnings to errors.  The transition is painful and
simple maintenance sometimes annoying, but the long term benefits, I
believe, are justified.  I find the most annoying is gcc's warning of
unreferenced variables used for debugging.

Jim Starkey, AmorphousDB, LLC

[Mark Rotteveel]

When and where are the methods of perf.cpp used?

Specifically, the method:

static int perf_format(const P* before, const P* after, const SCHAR*
string, SCHAR* buffer, SSHORT* buf_len)

This method seems to be a haven for potential buffer overflows, as it
will happily write into buffer, even if no buf_len or a zero or negative
buf_len was provided.

However, I couldn't find code that actually called it outside of other
methods in perf.cpp itself that simply forward calls to it.

Mark

[Alex Peshkoff]

On 4/24/25 13:07, 'Mark Rotteveel' via firebird-devel wrote:

When and where are the methods of perf.cpp used?

Specifically, the method:

static int perf_format(const P* before, const P* after, const SCHAR* string, SCHAR* buffer, SSHORT* buf_len)

This method seems to be a haven for potential buffer overflows, as it will happily write into buffer, even if no buf_len or a zero or negative buf_len was provided.

However, I couldn't find code that actually called it outside of other methods in perf.cpp itself that simply forward calls to it.

As expected for static function.

[Mark Rotteveel]

What do you mean? It is expected for a static function not to get
called? And I also mean those methods that call it don't seem to get
called in a way that has any effect.

That static int perf_format(const P* before, const P* after, const
SCHAR* string, SCHAR* buffer, SSHORT* buf_len) gets called by:

* static void perf_report(const P* before, const P* after, SCHAR*
buffer, SSHORT* buf_len)
* API_ROUTINE perf_format(const PERF* before, const PERF* after, const

SCHAR* string, SCHAR* buffer, SSHORT* buf_len)

* int API_ROUTINE perf64_format(const PERF64* before, const PERF64*

after, const SCHAR* string, SCHAR* buffer, SSHORT* buf_len)

I can't find anything calling those perf_format/perf64_format methods.

and that perf_report method is called by:

* void API_ROUTINE perf_report(const PERF* before, const PERF* after,
SCHAR* buffer, SSHORT* buf_len)
* void API_ROUTINE perf64_report(const PERF64* before, const PERF64*
after, SCHAR* buffer, SSHORT* buf_len)

and those don't seem to get called either.

Mark
--
Mark Rotteveel

[Vlad Khorsun]

When and where are the methods of perf.cpp used?

Specifically, the method:

static int perf_format(const P* before, const P* after, const SCHAR*
string, SCHAR* buffer, SSHORT* buf_len)

  It is part of non-public API used by isql only (?) before v3, where it was replaced by 

new OO API IUtil::getPerfCounters()

Regards,

Vlad

[Mark Rotteveel]

If it's no longer used, shouldn't it be removed?

Mark
--
Mark Rotteveel

[Mark Rotteveel]

Or I guess it's part of fbclient, so older ISQL versions loading a newer
fbclient still need it.

Mark
--
Mark Rotteveel

[Vlad Khorsun]

Mark Rotteveel:

  They are exported from firebird.dll and could be used by old isql, for example. 

However, I don't consider it as a strong reason to keep them.

Regards,

Vlad

[Mark Rotteveel]

It could be rewritten so absent or non-positive buffer lengths return
immediately, and so the rest doesn't actually ignore the length.

That might result in some reduced functionality if older ISQL version
actually call it with an absent or non-positive length, but that is OK I
think.

Mark
--
Mark Rotteveel

[Mark Rotteveel]

On 24/04/2025 13:26, 'Mark Rotteveel' via firebird-devel wrote:
> It could be rewritten so absent or non-positive buffer lengths return
> immediately, and so the rest doesn't actually ignore the length.
>
> That might result in some reduced functionality if older ISQL version
> actually call it with an absent or non-positive length, but that is OK I
> think.

I was looking at it again, and I think it might just be better to
replace the entire function body with `return 0;` (in `perf_format` in
perf.cpp). Any objections?

Mark
--
Mark Rotteveel

[Vlad Khorsun]

14.05.2025 9:40, 'Mark Rotteveel' via firebird-devel:

None.

Regards,
Vlad

[Mark Rotteveel]

(I already did it in my pull request for replacing sprintf ;)

Mark
--
Mark Rotteveel

[Vlad Khorsun]

15.05.2025 19:57, 'Mark Rotteveel' via firebird-devel:

Review is in progress and hasn't reached that file yet ;)

Regards,
Vlad

[Mark Rotteveel]

It is the very last file of that PR :)

Mark
--
Mark Rotteveel