Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v19]

[Michael Hall]

> On Dec 27, 2022, at 1:43 PM, Xue-Lei Andrew Fan  wrote:
> 
> On Tue, 27 Dec 2022 14:40:36 GMT, Christoph  wrote:
> 
>> ERROR: Build failed for target 'run-test-tier1' in configuration 
>> 'macosx-aarch64-server-release' (exit code 2) 
>> 
> [JDK-8299378](https://bugs.openjdk.org/browse/JDK-8299378) was filled to 
> track the issue.  Thanks!
> 
> -
> 
> PR: https://git.openjdk.org/jdk/pull/5

Thank you for providing the tracking issue.

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v19]

[Xue-Lei Andrew Fan]

On Tue, 27 Dec 2022 14:40:36 GMT, Christoph  wrote:

> ERROR: Build failed for target 'run-test-tier1' in configuration 
> 'macosx-aarch64-server-release' (exit code 2) 
> 
[JDK-8299378](https://bugs.openjdk.org/browse/JDK-8299378) was filled to track 
the issue.  Thanks!

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v19]

[Christoph]

On Thu, 8 Dec 2022 19:41:16 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 24 additional 
> commits since the last revision:
> 
>  - adlc update per review
>  - Merge
>  - update on review feedback
>  - comment for snprintf_checked
>  - use checked snprintf for adlc
>  - use checked snprintf
>  - no check on adlc
>  - revert use of assert
>  - extra sizeof typo
>  - more size_t updare for windows build
>  - ... and 14 more: https://git.openjdk.org/jdk/compare/3a3bbe53...49bb58fd

I just stumbled across this issue as well, make images did succeed, only the 
test is now failing with the same error
mac os 13.1 with XCode 14.2 on macbook M1 pro 
commit: 
11fd651ab1820770e3c65cd49589416098987a87 


ERROR: Build failed for target 'run-test-tier1' in configuration 
'macosx-aarch64-server-release' (exit code 2) 

=== Output from failing command(s) repeated here ===
* For target 
support_test_hotspot_jtreg_native_support_libAsyncExceptionOnMonitorEnter_libAsyncExceptionOnMonitorEnter.o:

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v19]

[Michael Hall]

> On Dec 9, 2022, at 11:57 AM, Laurent Bourgès  wrote:
> 
> On Thu, 8 Dec 2022 19:41:16 GMT, Xue-Lei Andrew Fan  
> wrote:
> 
>>> Hi,
>>> 
>>> May I have this update reviewed?
>>> 
>>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>>> use of it causing building failure.  The build could pass if warnings are 
>>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>>> could be used.
>>> 
>>> Thanks,
>>> Xuelei
>> 
>> Xue-Lei Andrew Fan has updated the pull request with a new target base due 
>> to a merge or a rebase. The incremental webrev excludes the unrelated 
>> changes brought in by the merge/rebase. The pull request contains 24 
>> additional commits since the last revision:
>> 
>> - adlc update per review
>> - Merge
>> - update on review feedback
>> - comment for snprintf_checked
>> - use checked snprintf for adlc
>> - use checked snprintf
>> - no check on adlc
>> - revert use of assert
>> - extra sizeof typo
>> - more size_t updare for windows build
>> - ... and 14 more: https://git.openjdk.org/jdk/compare/2309602b...49bb58fd
> 
> LGTM, build on macos 13 + xcode 14.1 (x64): OK
> 
> I run successfully few applications with built jdk image...
> 
> 
> Configuration summary:
> * Name:   macosx-x86_64-server-release
> * Debug level:release
> * HS debug level: product
> * JVM variants:   server
> * JVM features:   server: 'cds compiler1 compiler2 dtrace epsilongc g1gc jfr 
> jni-check jvmci jvmti management parallelgc serialgc services shenandoahgc 
> vm-structs zgc' 
> * OpenJDK target: OS: macosx, CPU architecture: x86, address length: 64
> * Version string: 20-internal-adhoc.jmmc.jdk-gh (20-internal)
> * Source date:1670605929 (2022-12-09T17:12:09Z)
> 
> Tools summary:
> * Boot JDK:   openjdk version "20-ea" 2023-03-21 OpenJDK Runtime 
> Environment (build 20-ea+26-2022) OpenJDK 64-Bit Server VM (build 
> 20-ea+26-2022, mixed mode, sharing) (at 
> /Users/jmmc/apps/jdk-20.jdk/Contents/Home)
> * Toolchain:  clang (clang/LLVM from Xcode 14.1)
> * Sysroot:
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk
> * C Compiler: Version 14.0.0 (at /usr/bin/clang)
> * C++ Compiler:   Version 14.0.0 (at /usr/bin/clang++)
> 
> -
> 
> PR: https://git.openjdk.org/jdk/pull/5

Fwiw, Where I seem to differ…

macOS 12.6 

* Version string: 20-internal-adhoc.mjh.jdk (20-internal)
* Source date:1668643046 (2022-11-16T23:57:26Z)

* Boot JDK:   openjdk version "19" 2022-09-20 OpenJDK Runtime Environment 
(build 19+36-2238) OpenJDK 64-Bit Server VM (build 19+36-2238, mixed mode, 
sharing) (at /Library/Java/JavaVirtualMachines/jdk-19.jdk/Contents/Home)

But it did build without the cmstypes errors.
Attempting tier 1 tests showed an sprintf deprecation error as follows…

=== Output from failing command(s) repeated here ===
* For target 
support_test_hotspot_jtreg_native_support_libAsyncExceptionOnMonitorEnter_libAsyncExceptionOnMonitorEnter.o:
/Users/mjh/Documents/GitHub/jdk/test/hotspot/jtreg/runtime/Thread/libAsyncExceptionOnMonitorEnter.cpp:37:3:
 error: '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. 
[-Werror,-Wdeprecated-declarations]
  sprintf(name, "MyRawMonitor");
  ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk/usr/include/stdio.h:188:1:
 note: 'sprintf' has been explicitly marked deprecated here
__deprecated_msg("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.")
^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk/usr/include/sys/cdefs.h:215:48:
 note: expanded from macro '__deprecated_msg'
#define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))
  ^
1 error generated.

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v19]

[Laurent Bourgès]

On Thu, 8 Dec 2022 19:41:16 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 24 additional 
> commits since the last revision:
> 
>  - adlc update per review
>  - Merge
>  - update on review feedback
>  - comment for snprintf_checked
>  - use checked snprintf for adlc
>  - use checked snprintf
>  - no check on adlc
>  - revert use of assert
>  - extra sizeof typo
>  - more size_t updare for windows build
>  - ... and 14 more: https://git.openjdk.org/jdk/compare/2309602b...49bb58fd

LGTM, build on macos 13 + xcode 14.1 (x64): OK

I run successfully few applications with built jdk image...


Configuration summary:
* Name:   macosx-x86_64-server-release
* Debug level:release
* HS debug level: product
* JVM variants:   server
* JVM features:   server: 'cds compiler1 compiler2 dtrace epsilongc g1gc jfr 
jni-check jvmci jvmti management parallelgc serialgc services shenandoahgc 
vm-structs zgc' 
* OpenJDK target: OS: macosx, CPU architecture: x86, address length: 64
* Version string: 20-internal-adhoc.jmmc.jdk-gh (20-internal)
* Source date:1670605929 (2022-12-09T17:12:09Z)

Tools summary:
* Boot JDK:   openjdk version "20-ea" 2023-03-21 OpenJDK Runtime 
Environment (build 20-ea+26-2022) OpenJDK 64-Bit Server VM (build 
20-ea+26-2022, mixed mode, sharing) (at 
/Users/jmmc/apps/jdk-20.jdk/Contents/Home)
* Toolchain:  clang (clang/LLVM from Xcode 14.1)
* Sysroot:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.sdk
* C Compiler: Version 14.0.0 (at /usr/bin/clang)
* C++ Compiler:   Version 14.0.0 (at /usr/bin/clang++)

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Xue-Lei Andrew Fan]

On Thu, 8 Dec 2022 17:19:56 GMT, Kim Barrett  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   update on review feedback
>
> src/hotspot/share/adlc/formssel.cpp line 28:
> 
>> 26: #include "adlc.hpp"
>> 27: 
>> 28: #define remaining_buflen(buffer, position) (sizeof(buffer) - (position - 
>> buffer))
> 
> Consider additional parenthesis, e.g. `((position) - (buffer))`.

It makes sense to me.  Thanks!

> src/hotspot/share/adlc/formssel.cpp line 1538:
> 
>> 1536:   }
>> 1537:   // Add predicate to working buffer
>> 1538:   snprintf_checked(s, remaining_buflen(buf, s), 
>> "/*%s*/(",(char*)i._key);
> 
> [pre-existing] the result from this snprintf could be used instead of strlen 
> in the next line.

I'm not very sure of why "+=" is used yet.  I would like to leave as it is.

> src/hotspot/share/adlc/output_c.cpp line 546:
> 
>> 544: char* args = new char [36 + 1];
>> 545: 
>> 546: snprintf_checked(args, 37, "0x%x, 0x%x, %u",
> 
> Instead of 37, consider `36 + 1` to be clearly from the line above.  Or give 
> that value a name.

Updated to use 36 + 1.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v19]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 24 additional commits 
since the last revision:

 - adlc update per review
 - Merge
 - update on review feedback
 - comment for snprintf_checked
 - use checked snprintf for adlc
 - use checked snprintf
 - no check on adlc
 - revert use of assert
 - extra sizeof typo
 - more size_t updare for windows build
 - ... and 14 more: https://git.openjdk.org/jdk/compare/bc6e44f6...49bb58fd

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/c391535c..49bb58fd

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=18
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=17-18

  Stats: 67182 lines in 1253 files changed: 30641 ins; 21894 del; 14647 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Kim Barrett]

On Wed, 7 Dec 2022 21:25:11 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update on review feedback

Looks good.  A couple of minor nits that you can address or not.

src/hotspot/share/adlc/formssel.cpp line 28:

> 26: #include "adlc.hpp"
> 27: 
> 28: #define remaining_buflen(buffer, position) (sizeof(buffer) - (position - 
> buffer))

Consider additional parenthesis, e.g. `((position) - (buffer))`.

src/hotspot/share/adlc/formssel.cpp line 1538:

> 1536:   }
> 1537:   // Add predicate to working buffer
> 1538:   snprintf_checked(s, remaining_buflen(buf, s), 
> "/*%s*/(",(char*)i._key);

[pre-existing] the result from this snprintf could be used instead of strlen in 
the next line.

src/hotspot/share/adlc/output_c.cpp line 546:

> 544: char* args = new char [36 + 1];
> 545: 
> 546: snprintf_checked(args, 37, "0x%x, 0x%x, %u",

Instead of 37, consider `36 + 1` to be clearly from the line above.  Or give 
that value a name.

-

Marked as reviewed by kbarrett (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Laurent Bourgès]

On Wed, 7 Dec 2022 21:25:11 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update on review feedback

I will try building openjdk20 with this patch tonight on mbp 2015, macos 13, 
xcode 14...

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Thomas Stuefe]

On Wed, 7 Dec 2022 21:25:11 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update on review feedback

Good from my side. But someone else should look as well. It's fiddly stuff.

-

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Michael Hall]

> On Dec 7, 2022, at 9:23 PM, Xue-Lei Andrew Fan  wrote:
> 
> On Wed, 7 Dec 2022 21:25:11 GMT, Xue-Lei Andrew Fan  > wrote:
> 
>>> Hi,
>>> 
>>> May I have this update reviewed?
>>> 
>>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>>> use of it causing building failure.  The build could pass if warnings are 
>>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>>> could be used.
>>> 
>>> Thanks,
>>> Xuelei
>> 
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>  update on review feedback
> 
>> With the exception of these errors in cmstypes.c
>> 
>> /Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:3441:132:
>>  error: parameter 'SizeOfTag' set but not used 
>> [-Werror,-Wunused-but-set-parameter] void 
>> *Type_ProfileSequenceId_Read(struct _cms_typehandler_struct* self, 
>> cmsIOHANDLER* io, cmsUInt32Number* nItems, cmsUInt32Number SizeOfTag) ^ 
>> /Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:5137:125:
>>  error: parameter 'SizeOfTag' set but not used 
>> [-Werror,-Wunused-but-set-parameter] void *Type_Dictionary_Read(struct 
>> _cms_typehandler_struct* self, cmsIOHANDLER* io, cmsUInt32Number* nItems, 
>> cmsUInt32Number SizeOfTag) ^ 2 errors generated.
>> 
>> I had seen this sometime back. The same workaround of adding?
>> 
>> cmsUNUSED_PARAMETER(SizeOfTag); // mjh
>> 
>> To the two methods, which I had noticed included elsewhere in the code, 
>> still appears to work.
>> 
> 
> The SizeOfTag issue was tracked with 
> https://bugs.openjdk.org/browse/JDK-8283221 
> .

Sorry, I should of mentioned that I had come across this bug report the first 
time I had this issue. I don’t think it was closed at the time. The 
--disable-warnings-as-errors seemed the only suggested workaround at the time. 
I thought mine was better. I’m not very familiar with these things but I assume 
that the fix for that bug hasn’t been merged into your pull request yet, but 
will be at some point.

> 
>> I first noticed the sprintf issue in later releases of Xcode 13. It isn?t 
>> just Xcode 14.
>> 
> 
> In the [Apple Developer 
> Documentation](https://developer.apple.com/documentation/kernel/1441083-sprintf
>  ), there 
> is a note for sprintf, "macOS 10.12–10.13.1 Deprecated".  It looks like that 
> deprecation was backported.

I installed an old Xcode (13.1) version. Which wasn’t a very easy process and 
probably only was possible since I have an Apple developer account to get the 
download. The next time I had gone into Xcode it forced an update to 14. I have 
continued to use the 13.1 to allow jdk builds until trying yours with 14.1. It 
worked as indicated.

Thanks for the reply.
 
> 
> -
> 
> PR: https://git.openjdk.org/jdk/pull/5 
> 

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Xue-Lei Andrew Fan]

On Wed, 7 Dec 2022 21:25:11 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update on review feedback

> With the exception of these errors in cmstypes.c
> 
> /Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:3441:132:
>  error: parameter 'SizeOfTag' set but not used 
> [-Werror,-Wunused-but-set-parameter] void *Type_ProfileSequenceId_Read(struct 
> _cms_typehandler_struct* self, cmsIOHANDLER* io, cmsUInt32Number* nItems, 
> cmsUInt32Number SizeOfTag) ^ 
> /Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:5137:125:
>  error: parameter 'SizeOfTag' set but not used 
> [-Werror,-Wunused-but-set-parameter] void *Type_Dictionary_Read(struct 
> _cms_typehandler_struct* self, cmsIOHANDLER* io, cmsUInt32Number* nItems, 
> cmsUInt32Number SizeOfTag) ^ 2 errors generated.
> 
> I had seen this sometime back. The same workaround of adding?
> 
> cmsUNUSED_PARAMETER(SizeOfTag); // mjh
> 
> To the two methods, which I had noticed included elsewhere in the code, still 
> appears to work.
> 

The SizeOfTag issue was tracked with 
https://bugs.openjdk.org/browse/JDK-8283221.

> I first noticed the sprintf issue in later releases of Xcode 13. It isn?t 
> just Xcode 14.
> 

In the [Apple Developer 
Documentation](https://developer.apple.com/documentation/kernel/1441083-sprintf),
 there is a note for sprintf, "macOS 10.12–10.13.1 Deprecated".  It looks like 
that deprecation was backported.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Xue-Lei Andrew Fan]

On Wed, 7 Dec 2022 08:46:45 GMT, Thomas Stuefe  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   comment for snprintf_checked
>
> src/hotspot/share/adlc/output_c.cpp line 46:
> 
>> 44:   va_end(args);
>> 45:   assert(result >= 0, "snprintf error");
>> 46:   assert(static_cast(result) < len, "snprintf truncated");
> 
> Question, what is this assert? The standard CRT assert? Will it fire always, 
> or just in debug?

The assert is defined in adlc.hpp, as follow:
`#define assert(cond, msg) { if (!(cond)) { fprintf(stderr, "assert fails %s 
%d: %s\n", __FILE__, __LINE__, msg); abort(); }}
`

I think it will fire always.

> src/hotspot/share/runtime/os.cpp line 102:
> 
>> 100: }
>> 101: 
>> 102: int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {
> 
> Can you please assert for buffer len > 0 and < some reasonable max, e.g. 1 
> gb? Protects us against neg overflows and other errors.

I may be hesitate for the checking. The behavior maybe unclear if a size_t 
value could be negative and if it is possible the max limit is too small in 
practice.  If it is necessary, the svnprintf() could do that instead.

> src/hotspot/share/runtime/os.cpp line 108:
> 
>> 106:   va_end(args);
>> 107:   assert(result >= 0, "os::snprintf error");
>> 108:   assert(static_cast(result) < len, "os::snprintf truncated");
> 
> Can you please provide a printout for the truncated string? Proposal:
> 
> ```assert(static_cast(result) < len, "os::snprintf truncated 
> ("%.200s...")", buf);```

I'm not sure if there is sensitive information in the buf.  Dumping the string 
may result in sensitive information leaking.

> src/hotspot/share/utilities/utf8.cpp line 227:
> 
>> 225: } else {
>> 226:   if (p + 6 >= end) break;  // string is truncated
>> 227:   os::snprintf_checked(p, 7, "\\u%04x", c);  // counting 
>> terminating zero in
> 
> I had to think a while until I was sure this is correct :-) 
> We are sure that c can never be > 0xFF (6 digits), right? But if that is 
> false, code had been wrong before too.

I think so as it used to work.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Xue-Lei Andrew Fan]

On Wed, 7 Dec 2022 16:00:04 GMT, Laurent Bourgès  wrote:

>> src/hotspot/os/bsd/attachListener_bsd.cpp line 260:
>> 
>>> 258:   // name ("load", "datadump", ...), and  is an argument
>>> 259:   int expected_str_count = 2 + AttachOperation::arg_count_max;
>>> 260:   const int max_len = (ver_str_len + 1) + 
>>> (AttachOperation::name_length_max + 1) +
>> 
>> This makes `max_len` a runtime variable, before it was a compile time 
>> constant. Below, we use it to create the buf array. IIRC creating an array 
>> with a variable runtime length is a non-standard compiler extension. I am 
>> surprised this even builds.
>
> better to keep sizeof(ver_str) here (const)

Updated to use sizeof.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v18]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  update on review feedback

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/c7dd001b..c391535c

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=17
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=16-17

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Laurent Bourgès]

On Wed, 7 Dec 2022 08:35:52 GMT, Thomas Stuefe  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   comment for snprintf_checked
>
> src/hotspot/os/bsd/attachListener_bsd.cpp line 260:
> 
>> 258:   // name ("load", "datadump", ...), and  is an argument
>> 259:   int expected_str_count = 2 + AttachOperation::arg_count_max;
>> 260:   const int max_len = (ver_str_len + 1) + 
>> (AttachOperation::name_length_max + 1) +
> 
> This makes `max_len` a runtime variable, before it was a compile time 
> constant. Below, we use it to create the buf array. IIRC creating an array 
> with a variable runtime length is a non-standard compiler extension. I am 
> surprised this even builds.

better to keep sizeof(ver_str) here (const)

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Michael Hall]

> On Dec 7, 2022, at 3:41 AM, Thomas Stuefe  wrote:
> 
>  am surprised this even builds.

It builds for me…

* Toolchain:  clang (clang/LLVM from Xcode 14.1)

With the exception of these errors in cmstypes.c

/Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:3441:132:
 error: parameter 'SizeOfTag' set but not used 
[-Werror,-Wunused-but-set-parameter]
void *Type_ProfileSequenceId_Read(struct _cms_typehandler_struct* self, 
cmsIOHANDLER* io, cmsUInt32Number* nItems, cmsUInt32Number SizeOfTag)

   ^
/Users/mjh/Documents/GitHub/jdk/src/java.desktop/share/native/liblcms/cmstypes.c:5137:125:
 error: parameter 'SizeOfTag' set but not used 
[-Werror,-Wunused-but-set-parameter]
void *Type_Dictionary_Read(struct _cms_typehandler_struct* self, cmsIOHANDLER* 
io, cmsUInt32Number* nItems, cmsUInt32Number SizeOfTag)

^
2 errors generated.

I had seen this sometime back. The same workaround of adding…

cmsUNUSED_PARAMETER(SizeOfTag);// mjh

To the two methods, which I had noticed included elsewhere in the code, still 
appears to work.

I first noticed the sprintf issue in later releases of Xcode 13. It isn’t just 
Xcode 14.

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Thomas Stuefe]

On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   comment for snprintf_checked

I looked through all the code and found only some minor issues. I would 
appreciate more eyes on this, though.

I still think this would have been less work (for author and reviewers) had we 
converted the code to stringStream right away in the hotspot. stringStream 
takes care of a lot of this boilerplate stuff.

src/hotspot/os/bsd/attachListener_bsd.cpp line 260:

> 258:   // name ("load", "datadump", ...), and  is an argument
> 259:   int expected_str_count = 2 + AttachOperation::arg_count_max;
> 260:   const int max_len = (ver_str_len + 1) + 
> (AttachOperation::name_length_max + 1) +

This makes `max_len` a runtime variable, before it was a compile time constant. 
Below, we use it to create the buf array. IIRC creating an array with a 
variable runtime length is a non-standard compiler extension. I am surprised 
this even builds.

src/hotspot/share/adlc/output_c.cpp line 46:

> 44:   va_end(args);
> 45:   assert(result >= 0, "snprintf error");
> 46:   assert(static_cast(result) < len, "snprintf truncated");

Question, what is this assert? The standard CRT assert? Will it fire always, or 
just in debug?

src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp line 311:

> 309:   for (int i = 0; i < len ; i++) {
> 310: VMStructEntry vmField = JVMCIVMStructs::localHotSpotVMStructs[i];
> 311: const size_t name_buf_size = strlen(vmField.typeName) + 
> strlen(vmField.fieldName) + 3 /* "::" */;

nit, could you make this "+ 2 + 1" instead? That makes it a bit clearer that 
the last byte is for \0

src/hotspot/share/runtime/os.cpp line 102:

> 100: }
> 101: 
> 102: int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {

Can you please assert for buffer len > 0 and < some reasonable max, e.g. 1 gb? 
Protects us against neg overflows and other errors.

src/hotspot/share/runtime/os.cpp line 108:

> 106:   va_end(args);
> 107:   assert(result >= 0, "os::snprintf error");
> 108:   assert(static_cast(result) < len, "os::snprintf truncated");

Can you please provide a printout for the truncated string? Proposal:

```assert(static_cast(result) < len, "os::snprintf truncated 
("%.200s...")", buf);```

src/hotspot/share/utilities/utf8.cpp line 227:

> 225: } else {
> 226:   if (p + 6 >= end) break;  // string is truncated
> 227:   os::snprintf_checked(p, 7, "\\u%04x", c);  // counting terminating 
> zero in

I had to think a while until I was sure this is correct :-) 
We are sure that c can never be > 0xFF (6 digits), right? But if that is 
false, code had been wrong before too.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Laurent Bourgès]

On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   comment for snprintf_checked

I will have a look, I am affected too

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Xue-Lei Andrew Fan]

On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   comment for snprintf_checked

Could I have the last update reviewed?

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Yichen Yan]

On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   comment for snprintf_checked

Any update on this? xcode 14 build has been broken for a while.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Xue-Lei Andrew Fan]

On Tue, 29 Nov 2022 07:57:36 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   comment for snprintf_checked

Please review the last update, and hopefully we are close to an agreement.  
Thanks!

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v12]

[Xue-Lei Andrew Fan]

On Sun, 27 Nov 2022 07:57:46 GMT, Thomas Stuefe  wrote:

> How about renaming the existing os::snprintf to something like 
> os::snprintf_unchecked, make os::snprintf the checked version, ...

The name `snprintf` may implies the function in C.  For that purpose, I may use 
a name different from`snprintf`, but I have no idea what it could be.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v12]

[Xue-Lei Andrew Fan]

On Sun, 27 Nov 2022 07:57:46 GMT, Thomas Stuefe  wrote:

> Given all the near-duplicated checking of os::snprintf results, I think there 
> is a place for a helper function to package this up. 

Thank you for the suggestion.  Updated to use snprintf_checked.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v17]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  comment for snprintf_checked

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/d1a48254..c7dd001b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=16
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=15-16

  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v16]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  use checked snprintf for adlc

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/4143f51e..d1a48254

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=15
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=14-15

  Stats: 43 lines in 7 files changed: 12 ins; 1 del; 30 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v15]

[Xue-Lei Andrew Fan]

On Mon, 28 Nov 2022 07:48:23 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   use checked snprintf

Please hold on the review.  I run into weird issues while using checked 
snprintf in `adlc`.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v15]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  use checked snprintf

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/6d91a6d7..4143f51e

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=14
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=13-14

  Stats: 30 lines in 13 files changed: 0 ins; 0 del; 30 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v14]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  no check on adlc

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/ee72fb50..6d91a6d7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=13
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=12-13

  Stats: 3 lines in 2 files changed: 1 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v12]

[Thomas Stuefe]

On Tue, 22 Nov 2022 08:02:51 GMT, Kim Barrett  wrote:

> Given all the near-duplicated checking of os::snprintf results, I think there 
> is a place for a helper function to package this up. Maybe something like
> 
> ```
> // in class os
> // Performs snprintf and asserts the result is non-negative (so there was not
> // an encoding error) and that the output was not truncated.
> static int snprintf_checked(char* buf, size_t len, const char* fmt, ...) 
> ATTRIBUTE_PRINTF(3, 4);
> 
> // in runtime/os.cpp
> int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {
>   va_list args;
>   va_start(args, fmt);
>   int result = os::vsnprintf(buf, len, fmt, args);
>   va_end(args);
>   assert(result >= 0, "os::snprintf error");
>   assert(static_cast(result) < size, "os::snprintf truncated");
>   return result;
> }
> ```
> 
> (I keep waffling over whether the truncation check should be an assert or a 
> guarantee.)
> 
> I've not yet gone through all the changes yet to consider which should do 
> that checking and which should do something different, such as permitting 
> truncation.
> 
> I'm not wedded to that name; indeed, I don't like it that much, as it's kind 
> of inconveniently long. There's a temptation to have os::snprintf forbid 
> truncation and a different function that allows it, but that would require 
> careful auditing of all pre-existing uses of os::snprintf too, so no.

How about renaming the existing os::snprintf to something like 
os::snprintf_unchecked, make os::snprintf the checked version, then, in 
separate RFEs, revert existing uses to the new API. When all uses of 
os::snprintf_unchecked are cleared up, remove it. 

That would make it possible to revert piecemeal while not racing with new uses 
of os::snprintf, since new callers will use the new checking API automatically.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v13]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  revert use of assert

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/4f80245f..ee72fb50

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=12
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=11-12

  Stats: 285 lines in 24 files changed: 15 ins; 181 del; 89 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v12]

[Kim Barrett]

On Fri, 18 Nov 2022 19:25:32 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   extra sizeof typo

Given all the near-duplicated checking of os::snprintf results, I think there
is a place for a helper function to package this up.  Maybe something like


// in class os
// Performs snprintf and asserts the result is non-negative (so there was not
// an encoding error) and that the output was not truncated.
static int snprintf_checked(char* buf, size_t len, const char* fmt, ...) 
ATTRIBUTE_PRINTF(3, 4);

// in runtime/os.cpp
int os::snprintf_checked(char* buf, size_t len, const char* fmt, ...) {
  va_list args;
  va_start(args, fmt);
  int result = os::vsnprintf(buf, len, fmt, args);
  va_end(args);
  assert(result >= 0, "os::snprintf error");
  assert(static_cast(result) < size, "os::snprintf truncated");
  return result;
}


(I keep waffling over whether the truncation check should be an assert or a 
guarantee.)

I've not yet gone through all the changes yet to consider which should do that
checking and which should do something different, such as permitting truncation.

I'm not wedded to that name; indeed, I don't like it that much, as it's kind
of inconveniently long.  There's a temptation to have os::snprintf forbid
truncation and a different function that allows it, but that would require
careful auditing of all pre-existing uses of os::snprintf too, so no.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[David Holmes]

On Mon, 21 Nov 2022 06:14:44 GMT, Xue-Lei Andrew Fan  wrote:

>> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
>> `os::snprintf`.
>> 
>> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918.
>> Regarding `os::snprintf` and `os::vsnprintf`, see 
>> https://bugs.openjdk.org/browse/JDK-8285506.
>> 
>> I think the only reason we haven't marked `::sprintf` and `::snprintf` 
>> forbidden
>> (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has gotten around
>> to dealing with it.  `::snprintf` in the list of candidates for
>> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been
>> marked.  But I don't see new bugs for the as-yet unmarked ones.
>> 
>> As a general note, as a reviewer my preference is against non-trivial and
>> persnickety code changes that are scattered all over the code base. For
>> something like this I'd prefer multiple more bite-sized changes that were
>> dealing with specific uses.  I doubt everyone agrees with me though.
>
> @kimbarrett, did you have further comments?  I'm going to integrate this 
> update this week.  Please let me know if you need more time.  Thanks!

@XueleiFan AFAICS only @tstuefe has reviewed the version where you check the 
return value, and as such you need a second reviewer for this nominal final 
version of the fix. Thanks.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Xue-Lei Andrew Fan]

On Sun, 13 Nov 2022 20:48:04 GMT, Kim Barrett  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
> `os::snprintf`.
> 
> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918.
> Regarding `os::snprintf` and `os::vsnprintf`, see 
> https://bugs.openjdk.org/browse/JDK-8285506.
> 
> I think the only reason we haven't marked `::sprintf` and `::snprintf` 
> forbidden
> (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has gotten around
> to dealing with it.  `::snprintf` in the list of candidates for
> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been
> marked.  But I don't see new bugs for the as-yet unmarked ones.
> 
> As a general note, as a reviewer my preference is against non-trivial and
> persnickety code changes that are scattered all over the code base. For
> something like this I'd prefer multiple more bite-sized changes that were
> dealing with specific uses.  I doubt everyone agrees with me though.

@kimbarrett, did you have further comments?  I'm going to integrate this update 
this week.  Please let me know if you need more time.  Thanks!

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v12]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  extra sizeof typo

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/c3da70cc..4f80245f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=11
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=10-11

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v11]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  more size_t updare for windows build

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/4fa31622..c3da70cc

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=10
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=09-10

  Stats: 8 lines in 1 file changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v10]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  fix size_t cast warning on windows

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/59a87dd1..4fa31622

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=09
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=08-09

  Stats: 24 lines in 2 files changed: 0 ins; 0 del; 24 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v9]

[Thomas Stuefe]

On Fri, 18 Nov 2022 06:42:24 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   size_t cast

Hi @XueleiFan,

the last version with the asserts looks fine to me. Thanks for your work!

Cheers, Thomas

-

Marked as reviewed by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v9]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  size_t cast

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/dcd7a8df..59a87dd1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=08
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=07-08

  Stats: 38 lines in 18 files changed: 0 ins; 0 del; 38 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v8]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  assert os::snprintf return value

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/f2158c8b..dcd7a8df

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=06-07

  Stats: 271 lines in 23 files changed: 181 ins; 1 del; 89 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Thomas Stuefe]

On Sun, 13 Nov 2022 22:55:52 GMT, Xue-Lei Andrew Fan  wrote:

>> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
>> `os::snprintf`.
>> 
>> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918.
>> Regarding `os::snprintf` and `os::vsnprintf`, see 
>> https://bugs.openjdk.org/browse/JDK-8285506.
>> 
>> I think the only reason we haven't marked `::sprintf` and `::snprintf` 
>> forbidden
>> (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has gotten around
>> to dealing with it.  `::snprintf` in the list of candidates for
>> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been
>> marked.  But I don't see new bugs for the as-yet unmarked ones.
>> 
>> As a general note, as a reviewer my preference is against non-trivial and
>> persnickety code changes that are scattered all over the code base. For
>> something like this I'd prefer multiple more bite-sized changes that were
>> dealing with specific uses.  I doubt everyone agrees with me though.
>
>> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
>> `os::snprintf`.
> 
> Updated to use os::snprintf, except the files under adlc where the 
> os::snptintf definition is not included.  The use of snprintf could be 
> cleaned up with existing code in the future.
> 
>> 
>> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. 
>> Regarding `os::snprintf` and `os::vsnprintf`, see 
>> https://bugs.openjdk.org/browse/JDK-8285506.
>> 
>> I think the only reason we haven't marked `::sprintf` and `::snprintf` 
>> forbidden (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has 
>> gotten around to dealing with it. `::snprintf` in the list of candidates for 
>> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been 
>> marked. But I don't see new bugs for the as-yet unmarked ones.
>> 
>> As a general note, as a reviewer my preference is against non-trivial and 
>> persnickety code changes that are scattered all over the code base. For 
>> something like this I'd prefer multiple more bite-sized changes that were 
>> dealing with specific uses. I doubt everyone agrees with me though.
> 
> It makes sense to me.  I'd better focus on the building issue in this PR.
> 
> Thank you for the review!

Hi @XueleiFan and @kimbarrett ,

I agree to the change if we, as Kim suggested, add assertions for truncation 
where we use the return value of snprintf.

I am not fully happy with the solution though, since printing is notoriously 
runtime-data dependent and runtime data can change in release builds. So we 
could have truncation at a customer that we never see in our tests with debug 
builds.

But seeing that this patch takes so long now and blocks the MacOS build, I 
don't want to block it. We can improve the code in follow up RFEs. These places 
would be a lot simpler with stringStream.

Cheers, Thomas

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Kim Barrett]

On Wed, 16 Nov 2022 09:06:22 GMT, Thomas Stuefe  wrote:

>> Updated to use the result from `os::snprtinf` in the new commit.
>
>> A result of -1 only occurs for an encoding error. An encoding error is only 
>> possible with multi-byte / wide characters. (See the definition of "encoding 
>> error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding 
>> errors, so our uses of snprintf never return -1.
> 
> Hi @kimbarrett,
> 
> I am not sure this was true. E.g. 
> https://stackoverflow.com/questions/65334245/what-is-an-encoding-error-for-sprintf-that-should-return-1
>  cites some cases where snprintf returns -1 that have nothing to do with 
> multibyte strings. Also, size=0  would return -1 according to SUSv2.
> 
> Note glibc differs and returns the number of chars it *would* have printed. 
> Which is also dangerous in a different way. If you use that number to update 
> the position, the position is not limited to buffer boundaries. So, I think 
> the result of os::snprintf should not be used to update buffer position, at 
> least not without checking.

SUSv2 is *ancient*. That got fixed in SUSv3 / POSIX.1-2001. From the Linux man
page for snprintf:

"Concerning the return value of snprintf(), SUSv2 and C99 contradict each
other: when snprintf() is called with size=0 then SUSv2 stipulates an
unspecified return value less than 1, while C99 allows str to be NULL in this
case, and gives the return value (as always) as the number of characters that
would have been written in case the output string has been large enough.
POSIX.1-2001 and later align their specification of snprintf() with C99."

For many/most places where we might use the return value, an assert of no
truncation would be appropriate. (Esp. likely in places where sprintf was
being used correctly.) Though there are probably places where something more
is needed.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v7]

[Thomas Stuefe]

On Wed, 16 Nov 2022 07:03:12 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   address review comments

src/hotspot/os/bsd/attachListener_bsd.cpp line 295:

> 293: char msg[32];
> 294: int msg_len = os::snprintf(msg, sizeof(msg), "%d\n", 
> ATTACH_ERROR_BADVERSION);
> 295: write_fully(s, msg, msg_len);

Assuming C99 behavior: safe but only because the buffer is large enough ("%d\n" 
needs at most 12 bytes, buffer is 32). Were it to overflow, msg_len would be 
larger than sizeof(msg) and we would probably end up reading beyond the message 
end in write_fully. So not really better than using sprintf+strlen.

src/hotspot/os/bsd/attachListener_bsd.cpp line 415:

> 413:   char msg[32];
> 414:   int msg_len = os::snprintf(msg, sizeof(msg), "%d\n", result);
> 415:   int rc = BsdAttachListener::write_fully(this->socket(), msg, msg_len);

same

src/hotspot/share/adlc/output_c.cpp line 217:

> 215: const PipeClassOperandForm *tmppipeopnd =
> 216: (const PipeClassOperandForm *)pipeclass->_localUsage[paramname];
> 217: templen += snprintf(&operand_stages[templen], operand_stages_size - 
> templen, "  stage_%s%c\n",

C99 Behavior: all these are probably safe but only because we never overstepped 
the buffer in the first place, the buffer size is pre-calculated. If it is 
incorrect and we have a truncation, subsequent writes will write beyond the 
buffer.

src/hotspot/share/classfile/javaClasses.cpp line 2527:

> 2525: 
> 2526:   // Print stack trace line in buffer
> 2527:   size_t buf_off = os::snprintf(buf, buf_size, "\tat %s.%s(", 
> klass_name, method_name);

Here, and in subsequent uses: assuming C99 behavior of snprintf, if we 
truncated in snprintf, buf_off will be > buffer size, (buf + buf_off) point 
beyond the buffer, (buf_size - buf_off) will overflow and become very large.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v7]

[Thomas Stuefe]

On Wed, 16 Nov 2022 07:03:12 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   address review comments

I don't think it is safe to use the return value of `os::snprintf` to update 
buffer positions.

`os::snprintf()` calls `os::vsnprintf()` which, at least on Posix, returns the 
return value of `vnsprintf(3)` verbatim. 

If native `vsnprintf(3)` conforms to SUSv2 [1], it will return <0 if the buffer 
size is zero. So for cases where we calculate the buffer size based on what was 
written, and we are just at the edge of the buffer, we would move the next 
write position backward.

But much worse: if the native `vsnprintf(3)` conforms to C99 (e.g. glibc, BSD 
libc) they return "number of characters (not including the terminating null 
character) which would have been written to buffer if bufsz was ignored". [2] 
So, if the buffer was too small and we truncated, and we use the return value 
to calculate the next write position, we will write outside the buffer 
boundaries.

Regardless of what behavior we have - C99 or SUSv2 - we cannot just use the 
return value to update the next write position without first checking the 
return value.

We could - and probably should - decide on C99 or SUSv2 behavior for all 
platforms, and modify `os::snprintf()` to provide that in an OS-independent 
way. But unless we decide on some mongrel of both C99 and SUSv2 behaviors, the 
problem remains.

Cheers, Thomas

[1] https://pubs.opengroup.org/onlinepubs/7908799/xsh/fprintf.html
[2] https://en.cppreference.com/w/c/io/fprintf

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Thomas Stuefe]

On Wed, 16 Nov 2022 05:45:34 GMT, Xue-Lei Andrew Fan  wrote:

>> A result of -1 only occurs for an encoding error.  An encoding error is only
>> possible with multi-byte / wide characters.  (See the definition of "encoding
>> error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding
>> errors, so our uses of snprintf never return -1.
>
> Updated to use the result from `os::snprtinf` in the new commit.

> A result of -1 only occurs for an encoding error. An encoding error is only 
> possible with multi-byte / wide characters. (See the definition of "encoding 
> error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding 
> errors, so our uses of snprintf never return -1.

Hi @kimbarrett,

I am not sure this was true. E.g. 
https://stackoverflow.com/questions/65334245/what-is-an-encoding-error-for-sprintf-that-should-return-1
 cites some cases where snprintf returns -1 that have nothing to do with 
multibyte strings. Also, size=0  would return -1 according to SUSv2.

Note glibc differs and returns the number of chars it *would* have printed. 
Which is also dangerous in a different way. If you use that number to update 
the position, the position is not limited to buffer boundaries. So, I think the 
result of os::snprintf should not be used to update buffer position, at least 
not without checking.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Lutz Schmidt]

On Wed, 16 Nov 2022 06:43:29 GMT, Xue-Lei Andrew Fan  wrote:

>> src/hotspot/share/utilities/utf8.cpp line 521:
>> 
>>> 519: } else {
>>> 520:   if (p + 6 >= end) break;  // string is truncated
>>> 521:   os::snprintf(p, 7, "\\u%04x", c);
>> 
>> This should be 6, or? We have 6 characters left before end, assuming end is 
>> exclusive.
>> 
>> Also, maybe use a named constant?
>
> If 6 is used, there is a output truncated warning and only 5 characters are 
> filed actually.  The terminating null/zero is counted in, I think.  To make 
> it easier to read, I added a comment.

For snprintf, all bytes written to the buffer (including the terminating \0) 
are counted. You have 6 bytes for character encoding ("\u") and one byte 
for \0. Code is correct.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Xue-Lei Andrew Fan]

On Tue, 15 Nov 2022 05:52:18 GMT, Thomas Stuefe  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   delete swp file
>
> src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 226:
> 
>> 224:   char buf[512];
>> 225:   os::snprintf(buf, sizeof(buf), "0x%02x:0x%x:0x%03x:%d", _cpu, 
>> _variant, _model, _revision);
>> 226:   if (_model2) os::snprintf(buf+strlen(buf), sizeof(buf) - strlen(buf), 
>> "(0x%03x)", _model2);
> 
> Here - and in several other places, where we construct a string from multiple 
> parts - the code would be a simpler with `stringStream`:
> 
> 
> char buf[512];
> stringStream ss(buf, sizeof(buf));
> ss.print("0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
> if (_model2) ss.print("(0x%03x)", _model2);
> _features_string = os::strdup(buf);
> 
> or, using `stringStream`s internal buffer:
> 
> 
> stringStream ss;
> ss.print("0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
> if (_model2) ss.print("(0x%03x)", _model2);
> _features_string = ss.base();
> 
> 
> No manual offset counting required.
> 
> I leave it up to you if you do it that way. The code here is correct as it is.

Glad to know that `stringStream` could make this block safer and easier.  I 
learned a lot from the reviewers of this PR.  It looks like we can also use 
stringStream for other files touched in this PR.  I would like to keep the 
update focusing on the simple replacing of `sprintf`.  I may file a new PR for 
the improvement by using `stringStream` shortly after.

> src/hotspot/share/classfile/javaClasses.cpp line 2562:
> 
>> 2560:   CompiledMethod* nm = method->code();
>> 2561:   if (WizardMode && nm != NULL) {
>> 2562: os::snprintf(buf + buf_off, buf_size - buf_off, "(nmethod " 
>> INTPTR_FORMAT ")", (intptr_t)nm);
> 
> I think you should update `buf_off` here, because now you overwrite the last 
> text part. Weird that no test caught that.
> 
> All this code here in javaClasses.cpp would benefit from using stringStream.

Oooops!  Good catch.  `buf_off` is updated in the new commit.  I think 
`stringStream` could be a better solution.  I will do it in a follow-up PR.

> src/hotspot/share/utilities/utf8.cpp line 521:
> 
>> 519: } else {
>> 520:   if (p + 6 >= end) break;  // string is truncated
>> 521:   os::snprintf(p, 7, "\\u%04x", c);
> 
> This should be 6, or? We have 6 characters left before end, assuming end is 
> exclusive.
> 
> Also, maybe use a named constant?

If 6 is used, there is a output truncated warning and only 5 characters are 
filed actually.  The terminating null/zero is counted in, I think.  To make it 
easier to read, I added a comment.

> src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_Ports.cpp line 
> 638:
> 
>> 636: return;
>> 637: }
>> 638: snprintf(channelName, 16, "Ch %d", ch);
> 
> Can we use a constant here instead of literal 16?

To be honest, I don't know the logic of the code yet.  I'm not sure how to name 
the literal 16 yet.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Xue-Lei Andrew Fan]

On Wed, 16 Nov 2022 04:55:17 GMT, Kim Barrett  wrote:

>> The problem with using the return value of os::snprintf() is that we need to 
>> handle the -1 case to prevent the position from running backward. Might be 
>> better to use stringStream instead, which should handle the -1 case 
>> transparently.
>
> A result of -1 only occurs for an encoding error.  An encoding error is only
> possible with multi-byte / wide characters.  (See the definition of "encoding
> error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding
> errors, so our uses of snprintf never return -1.

Updated to use the result from `os::snprtinf` in the new commit.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Xue-Lei Andrew Fan]

On Tue, 15 Nov 2022 07:04:38 GMT, Kim Barrett  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   delete swp file
>
> src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 226:
> 
>> 224:   char buf[512];
>> 225:   os::snprintf(buf, sizeof(buf), "0x%02x:0x%x:0x%03x:%d", _cpu, 
>> _variant, _model, _revision);
>> 226:   if (_model2) os::snprintf(buf+strlen(buf), sizeof(buf) - strlen(buf), 
>> "(0x%03x)", _model2);
> 
> Instead of using `strlen(buf)` (now called twice!) to get the number of 
> characters written, use the result of the first call to `os::snprintf`.

Good point!  Updated in the new commit.

> src/hotspot/os/bsd/attachListener_bsd.cpp line 251:
> 
>> 249: BsdAttachOperation* BsdAttachListener::read_request(int s) {
>> 250:   char ver_str[8];
>> 251:   os::snprintf(ver_str, sizeof(ver_str), "%d", ATTACH_PROTOCOL_VER);
> 
> We later use `strlen(ver_str)` where we could instead use the result of 
> `os::snprintf`.

I think it is safe to use the result of `os::snprintf` for the computation of 
max_len of the buf, isn't it?


-  const int max_len = (sizeof(ver_str) + 1) + 
(AttachOperation::name_length_max + 1) +
+  const int max_len = (ver_str_len + 1) + (AttachOperation::name_length_max + 
1) +
AttachOperation::arg_count_max*(AttachOperation::arg_length_max + 1);

> src/hotspot/os/bsd/attachListener_bsd.cpp line 414:
> 
>> 412:   // write operation result
>> 413:   char msg[32];
>> 414:   os::snprintf(msg, sizeof(msg), "%d\n", result);
> 
> Rather than using strlen(msg) in the next line, use the result from 
> os::snprintf.

Updated to use the result from os::snprtinf in the new commit.

> src/hotspot/share/classfile/javaClasses.cpp line 2532:
> 
>> 2530:   // Print module information
>> 2531:   if (module_name != NULL) {
>> 2532: buf_off = (int)strlen(buf);
> 
> `buf_off` could be the result of `os::snprintf` instead of calling `strlen`.

Updated to use the result of `os::snprintf` in the new commit.

> src/hotspot/share/code/dependencies.cpp line 780:
> 
>> 778:   }
>> 779: } else {
>> 780:   char xn[12]; os::snprintf(xn, sizeof(xn), "x%d", j);
> 
> Pre-existing very unusual formatting; put a line break between the statements.

Yes.  Updated in the new commit.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]

[Xue-Lei Andrew Fan]

On Mon, 14 Nov 2022 16:53:07 GMT, Lutz Schmidt  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   include missing os head file
>
> src/hotspot/share/adlc/output_c.cpp line 536:
> 
>> 534: int printed = snprintf(args, 37, "0x%x, 0x%x, %u",
>> 535:   resources_used, resources_used_exclusively, element_count);
>> 536: assert(printed <= 36, "overflow");
> 
> if snprintf works correctly (we rely on that), this assert will never fire.

Good point.  I removed the assert in the new commit.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v7]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  address review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/ca4ddcc4..f2158c8b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=05-06

  Stats: 24 lines in 6 files changed: 1 ins; 4 del; 19 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Kim Barrett]

On Tue, 15 Nov 2022 08:31:10 GMT, Thomas Stuefe  wrote:

>> src/hotspot/os/bsd/attachListener_bsd.cpp line 294:
>> 
>>> 292:   (atoi(buf) != ATTACH_PROTOCOL_VER)) {
>>> 293: char msg[32];
>>> 294: os::snprintf(msg, sizeof(msg), "%d\n", 
>>> ATTACH_ERROR_BADVERSION);
>> 
>> Rather than using `strlen(msg)` in the next line, use the result from 
>> `os::snprintf`.
>
> The problem with using the return value of os::snprintf() is that we need to 
> handle the -1 case to prevent the position from running backward. Might be 
> better to use stringStream instead, which should handle the -1 case 
> transparently.

A result of -1 only occurs for an encoding error.  An encoding error is only
possible with multi-byte / wide characters.  (See the definition of "encoding
error" in C99 7.19.3/14.) We don't use those, so there won't be any encoding
errors, so our uses of snprintf never return -1.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Lutz Schmidt]

On Mon, 14 Nov 2022 19:44:17 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   delete swp file

With the comments from others honoured, changes look good to me. 
I found just one, now obsolete, assert which you may want to delete.

-

Marked as reviewed by lucy (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]

[Lutz Schmidt]

On Mon, 14 Nov 2022 05:32:20 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   include missing os head file

src/hotspot/share/adlc/output_c.cpp line 536:

> 534: int printed = snprintf(args, 37, "0x%x, 0x%x, %u",
> 535:   resources_used, resources_used_exclusively, element_count);
> 536: assert(printed <= 36, "overflow");

if snprintf works correctly (we rely on that), this assert will never fire.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Thomas Stuefe]

On Tue, 15 Nov 2022 07:13:49 GMT, Kim Barrett  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   delete swp file
>
> src/hotspot/os/bsd/attachListener_bsd.cpp line 294:
> 
>> 292:   (atoi(buf) != ATTACH_PROTOCOL_VER)) {
>> 293: char msg[32];
>> 294: os::snprintf(msg, sizeof(msg), "%d\n", 
>> ATTACH_ERROR_BADVERSION);
> 
> Rather than using `strlen(msg)` in the next line, use the result from 
> `os::snprintf`.

The problem with using the return value of os::snprintf() is that we need to 
handle the -1 case to prevent the position from running backward. Might be 
better to use stringStream instead, which should handle the -1 case 
transparently.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Thomas Stuefe]

On Mon, 14 Nov 2022 19:44:17 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   delete swp file

Hi @XueleiFan,

good job, this looks like it was onerous work!

One issue I noticed, in ADLC only: we sometimes use the snprintf return value 
to update a position pointer, e.g. in adlc output_c.cpp; should snprintf return 
-1, we could run backwards and overstep the beginning of the buffer.

Totally up to you if you fix it, and whether as a follow-up RFE or here. If you 
do, the simplest way may be to add a little `stringStream`-like helper like 
this to ADLC:


class AdlcStringStream {
  char* const _buf; 
  const size_t _buflen;
  size_t _pos;
public:
  AdlcStringStream(char* out, size_t outlen) : _buf(out), _buflen(outlen), 
_pos(0) {}
  void print(const char* fmt, ...) {
if (_pos < _buflen) { 
  va_list ap;
  va_start (ap, fmt);
  int written = vsnprintf(_buf + _pos, _buflen - _pos, fmt, ap);
  va_end(ap);
  if (written > 0) {
_pos += written;
  }
}
  }
  const char* buf() const { return _buf; }
};

and use that instead. Way easier to read the code then. Optionally, the helper 
could even handle buffer allocation and destruction.

All other remarks inline. Small issues remain, but nothing drastic.

Cheers, Thomas

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 226:

> 224:   char buf[512];
> 225:   os::snprintf(buf, sizeof(buf), "0x%02x:0x%x:0x%03x:%d", _cpu, 
> _variant, _model, _revision);
> 226:   if (_model2) os::snprintf(buf+strlen(buf), sizeof(buf) - strlen(buf), 
> "(0x%03x)", _model2);

Here - and in several other places, where we construct a string from multiple 
parts - the code would be a simpler with `stringStream`:


char buf[512];
stringStream ss(buf, sizeof(buf));
ss.print("0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
if (_model2) ss.print("(0x%03x)", _model2);
_features_string = os::strdup(buf);

or, using `stringStream`s internal buffer:


stringStream ss;
ss.print("0x%02x:0x%x:0x%03x:%d", _cpu, _variant, _model, _revision);
if (_model2) ss.print("(0x%03x)", _model2);
_features_string = ss.base();


No manual offset counting required.

I leave it up to you if you do it that way. The code here is correct as it is.

src/hotspot/share/classfile/javaClasses.cpp line 2562:

> 2560:   CompiledMethod* nm = method->code();
> 2561:   if (WizardMode && nm != NULL) {
> 2562: os::snprintf(buf + buf_off, buf_size - buf_off, "(nmethod " 
> INTPTR_FORMAT ")", (intptr_t)nm);

I think you should update `buf_off` here, because now you overwrite the last 
text part. Weird that no test caught that.

All this code here in javaClasses.cpp would benefit from using stringStream.

src/hotspot/share/utilities/utf8.cpp line 521:

> 519: } else {
> 520:   if (p + 6 >= end) break;  // string is truncated
> 521:   os::snprintf(p, 7, "\\u%04x", c);

This should be 6, or? We have 6 characters left before end, assuming end is 
exclusive.

Also, maybe use a named constant?

src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_Ports.cpp line 638:

> 636: return;
> 637: }
> 638: snprintf(channelName, 16, "Ch %d", ch);

Can we use a constant here instead of literal 16?

-

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Kim Barrett]

On Mon, 14 Nov 2022 19:44:17 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   delete swp file

Mostly okay.  There are some places where the result from `os::snprintf` could 
be used instead of a later `strlen`.  Most of those are pre-existing (so could 
be considered for later cleanups), but in at least one case there was a new 
strlen call introduced, so making the code slightly worse.

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 226:

> 224:   char buf[512];
> 225:   os::snprintf(buf, sizeof(buf), "0x%02x:0x%x:0x%03x:%d", _cpu, 
> _variant, _model, _revision);
> 226:   if (_model2) os::snprintf(buf+strlen(buf), sizeof(buf) - strlen(buf), 
> "(0x%03x)", _model2);

Instead of using `strlen(buf)` (now called twice!) to get the number of 
characters written, use the result of the first call to `os::snprintf`.

src/hotspot/os/bsd/attachListener_bsd.cpp line 251:

> 249: BsdAttachOperation* BsdAttachListener::read_request(int s) {
> 250:   char ver_str[8];
> 251:   os::snprintf(ver_str, sizeof(ver_str), "%d", ATTACH_PROTOCOL_VER);

We later use `strlen(ver_str)` where we could instead use the result of 
`os::snprintf`.

src/hotspot/os/bsd/attachListener_bsd.cpp line 294:

> 292:   (atoi(buf) != ATTACH_PROTOCOL_VER)) {
> 293: char msg[32];
> 294: os::snprintf(msg, sizeof(msg), "%d\n", 
> ATTACH_ERROR_BADVERSION);

Rather than using `strlen(msg)` in the next line, use the result from 
`os::snprintf`.

src/hotspot/os/bsd/attachListener_bsd.cpp line 414:

> 412:   // write operation result
> 413:   char msg[32];
> 414:   os::snprintf(msg, sizeof(msg), "%d\n", result);

Rather than using strlen(msg) in the next line, use the result from 
os::snprintf.

src/hotspot/share/classfile/javaClasses.cpp line 2532:

> 2530:   // Print module information
> 2531:   if (module_name != NULL) {
> 2532: buf_off = (int)strlen(buf);

`buf_off` could be the result of `os::snprintf` instead of calling `strlen`.

src/hotspot/share/code/dependencies.cpp line 780:

> 778:   }
> 779: } else {
> 780:   char xn[12]; os::snprintf(xn, sizeof(xn), "x%d", j);

Pre-existing very unusual formatting; put a line break between the statements.

-

Changes requested by kbarrett (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v6]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  delete swp file

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/32e18955..ca4ddcc4

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=04-05

  Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]

[Xue-Lei Andrew Fan]

On Mon, 14 Nov 2022 10:21:07 GMT, Thomas Stuefe  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   include missing os head file
>
> src/hotspot/share/adlc/output_c.cpp line 2570:
> 
>> 2568: int idx = inst.operand_position_format(arg_name);
>> 2569: if (strcmp(arg_name, "constanttablebase") == 0) {
>> 2570:   ib += snprintf(ib, (buflen - (ib - idxbuf)), "  unsigned 
>> idx_%-5s = mach_constant_base_node_input(); \t// %s, \t%s\n",
> 
> Use sizeof(buffer) instead of buflen?
> Also, possibly using a helper macro like this:
> 
> 
> #define remaining_buflen(buffer, position) (sizeof(buffer) - (position - 
> buffer))
> 
> would make the code a bit easier on the eye. Or, if not a macro, an inline 
> helper function, that could assert also array boundaries.

Thanks for suggestion, which makes the code much easier to read.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v5]

[Phil Race]

On Mon, 14 Nov 2022 19:05:16 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   use helper macro

The single client change looks fine. I didn't look at the rest.

-

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v5]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  use helper macro

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/128bc806..32e18955

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=03-04

  Stats: 10 lines in 3 files changed: 4 ins; 0 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]

[Thomas Stuefe]

On Mon, 14 Nov 2022 05:32:20 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   include missing os head file

src/hotspot/share/adlc/output_c.cpp line 2570:

> 2568: int idx = inst.operand_position_format(arg_name);
> 2569: if (strcmp(arg_name, "constanttablebase") == 0) {
> 2570:   ib += snprintf(ib, (buflen - (ib - idxbuf)), "  unsigned idx_%-5s 
> = mach_constant_base_node_input(); \t// %s, \t%s\n",

Use sizeof(buffer) instead of buflen?
Also, possibly using a helper macro like this:


#define remaining_buflen(buffer, position) (sizeof(buffer) - (position - 
buffer))

would make the code a bit easier on the eye. Or, if not a macro, an inline 
helper function, that could assert also array boundaries.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]

[Andrew Haley]

On Mon, 14 Nov 2022 05:32:20 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   include missing os head file

Kim said:
> As a general note, as a reviewer my preference is against non-trivial and 
> persnickety code changes that are scattered all over the code base. For 
> something like this I'd prefer multiple more bite-sized changes that were 
> dealing with specific uses. I doubt everyone agrees with me though.

There's a lot of wisdom in what you say. It's far too easy to mess things up 
when doing cleanups for compiler warnings. Also, long patches never get enough 
reviewing.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Thomas Stuefe]

On Sun, 13 Nov 2022 22:55:52 GMT, Xue-Lei Andrew Fan  wrote:

> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
> `os::snprintf`.

I did not know this was our policy now. Sorry for giving the wrong advice. 
Maybe we should add this to the hotspot style guide since I'm probably not the 
only one not knowing this.

> 
> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. 
> Regarding `os::snprintf` and `os::vsnprintf`, see 
> https://bugs.openjdk.org/browse/JDK-8285506.
> 
> I think the only reason we haven't marked `::sprintf` and `::snprintf` 
> forbidden (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has 
> gotten around to dealing with it. `::snprintf` in the list of candidates for 
> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been 
> marked. But I don't see new bugs for the as-yet unmarked ones.
> 
> As a general note, as a reviewer my preference is against non-trivial and 
> persnickety code changes that are scattered all over the code base. For 
> something like this I'd prefer multiple more bite-sized changes that were 
> dealing with specific uses. I doubt everyone agrees with me though.

I agree with you. Makes backporting a bit easier too.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v4]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  include missing os head file

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/fe6893d5..128bc806

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v2]

[Xue-Lei Andrew Fan]

On Mon, 14 Nov 2022 01:51:32 GMT, David Holmes  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - use os::snprintf for desktop update
>>  - use os::snprintf
>
> src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_Ports.cpp line 
> 638:
> 
>> 636: return;
>> 637: }
>> 638: os::snprintf(channelName, 16, "Ch %d", ch);
> 
> You can't use this here - this is not hotspot code!

You are right.  Reverted to use `snprintf` for desktop update.  Thanks!

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v3]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  revert update for desktop

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/a66f58bf..fe6893d5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v2]

[David Holmes]

On Sun, 13 Nov 2022 22:55:30 GMT, Xue-Lei Andrew Fan  wrote:

>> Hi,
>> 
>> May I have this update reviewed?
>> 
>> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
>> use of it causing building failure.  The build could pass if warnings are 
>> disabled for codes that use sprintf method.  For the long run, the sprintf 
>> could be replaced with snprintf.  This patch is trying to check if snprintf 
>> could be used.
>> 
>> Thanks,
>> Xuelei
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - use os::snprintf for desktop update
>  - use os::snprintf

The hotspot changes seem okay using os::snprint. The adlc changes to use raw 
snprintf also seem okay for now - I'm not sure whether the platform differences 
for snprintf affect adlc.

The desktop change is wrong - you can't use os::snprintf there.

Thanks.

src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_Ports.cpp line 638:

> 636: return;
> 637: }
> 638: os::snprintf(channelName, 16, "Ch %d", ch);

You can't use this here - this is not hotspot code!

-

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Xue-Lei Andrew Fan]

On Sun, 13 Nov 2022 20:48:04 GMT, Kim Barrett  wrote:

> Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
> `os::snprintf`.

Updated to use os::snprintf, except the files under adlc where the os::snptintf 
definition is not included.  The use of snprintf could be cleaned up with 
existing code in the future.

> 
> Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918. 
> Regarding `os::snprintf` and `os::vsnprintf`, see 
> https://bugs.openjdk.org/browse/JDK-8285506.
> 
> I think the only reason we haven't marked `::sprintf` and `::snprintf` 
> forbidden (FORBID_C_FUNCTION) is there are a lot of uses, and nobody has 
> gotten around to dealing with it. `::snprintf` in the list of candidates for 
> https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been 
> marked. But I don't see new bugs for the as-yet unmarked ones.
> 
> As a general note, as a reviewer my preference is against non-trivial and 
> persnickety code changes that are scattered all over the code base. For 
> something like this I'd prefer multiple more bite-sized changes that were 
> dealing with specific uses. I doubt everyone agrees with me though.

It makes sense to me.  I'd better focus on the building issue in this PR.

Thank you for the review!

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14 [v2]

[Xue-Lei Andrew Fan]

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with two 
additional commits since the last revision:

 - use os::snprintf for desktop update
 - use os::snprintf

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/5/files
  - new: https://git.openjdk.org/jdk/pull/5/files/e4724c5f..a66f58bf

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=5&range=00-01

  Stats: 41 lines in 18 files changed: 0 ins; 0 del; 41 mod
  Patch: https://git.openjdk.org/jdk/pull/5.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/5/head:pull/5

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Kim Barrett]

On Fri, 11 Nov 2022 22:41:19 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Please don't add uses of `jio_snprintf` or `::snprintf` to hotspot. Use 
`os::snprintf`.

Regarding `jio_snprintf`, see https://bugs.openjdk.org/browse/JDK-8198918.
Regarding `os::snprintf` and `os::vsnprintf`, see 
https://bugs.openjdk.org/browse/JDK-8285506.

I think the only reason we haven't marked `::sprintf` and `::snprintf` forbidden
(FORBID_C_FUNCTION) is there are a lot of uses, and nobody has gotten around
to dealing with it.  `::snprintf` in the list of candidates for
https://bugs.openjdk.org/browse/JDK-8214976, some of which have already been
marked.  But I don't see new bugs for the as-yet unmarked ones.

As a general note, as a reviewer my preference is against non-trivial and
persnickety code changes that are scattered all over the code base. For
something like this I'd prefer multiple more bite-sized changes that were
dealing with specific uses.  I doubt everyone agrees with me though.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Thomas Stuefe]

On Sun, 13 Nov 2022 08:25:57 GMT, Xue-Lei Andrew Fan  wrote:

> > could you use `jio_snprintf` instead (see include/jvm_io.h)? That is what 
> > we usually do for snprintf. jio_snprintf hides platform particularities wrt 
> > snprintf.
> 
> Good to know that. Thank you!
> 
> While I was doing the replacement from `snprintf` to `jio_snprintf`, I 
> noticed a lot of existing use of `snprintf` in the files touched in this PR. 
> What do you think if we have a `snprintf` clean up in a followed PR?
> 
> ```
> hotspot $ find . -type f |xargs grep snprintf |grep -v jio_snprintf |wc   
>
>  2621895   26574
> ```

Hmm, possibly. We may look again at the exact reason why we use jio_snprintf. 
Maybe it is less important nowadays, with reduced platform number (no solaris) 
and Windows being more standard conform than it had been in the past.

Lets hear what others think.

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Xue-Lei Andrew Fan]

On Sun, 13 Nov 2022 07:50:43 GMT, Thomas Stuefe  wrote:

> could you use `jio_snprintf` instead (see include/jvm_io.h)? That is what we 
> usually do for snprintf. jio_snprintf hides platform particularities wrt 
> snprintf.
> 

Good to know that.  Thank you!

While I was doing the replacement from `snprintf` to `jio_snprintf`, I noticed 
a lot of existing use of `snprintf` in the files touched in this PR.  What do 
you think if we have a `snprintf` clean up in a followed PR?


hotspot $ find . -type f |xargs grep snprintf |grep -v jio_snprintf |wc 
 
 2621895   26574

-

PR: https://git.openjdk.org/jdk/pull/5

Re: RFR: 8296812: sprintf is deprecated in Xcode 14

[Thomas Stuefe]

On Fri, 11 Nov 2022 22:41:19 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> May I have this update reviewed?
> 
> The sprintf is deprecated in Xcode 14 because of security concerns, and the 
> use of it causing building failure.  The build could pass if warnings are 
> disabled for codes that use sprintf method.  For the long run, the sprintf 
> could be replaced with snprintf.  This patch is trying to check if snprintf 
> could be used.
> 
> Thanks,
> Xuelei

Hi @XueleiFan,

could you use `jio_snprintf` instead (see include/jvm_io.h)? That is what we 
usually do for snprintf. jio_snprintf hides platform particularities wrt 
snprintf.

Cheers, Thomas

-

PR: https://git.openjdk.org/jdk/pull/5