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

[Thomas Stuefe]

On Mon, 14 Nov 2022 19:44:17 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> 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/11115