Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed maybe-uninitialized warning in error creation. #744

Merged
merged 1 commit into from
Jun 29, 2024

Conversation

xeioex
Copy link
Contributor

@xeioex xeioex commented Jun 22, 2024

Ensuring that buf is always initialized in njs_throw_error_va() and njs_error_fmt_new(), by requiring fmt to always be non NULL.

This fixes GCC warnings like:
169 | njs_unicode_decode_t ctx;
| ^
In function ‘njs_utf8_length’,
inlined from ‘njs_error_new’ at src/njs_error.c:39:14,
inlined from ‘njs_throw_error_va’ at src/njs_error.c:69:5:
src/njs_utf8.h:141:12: error: ‘buf’ may be used uninitialized
[-Werror=maybe-uninitialized]
141 | return njs_utf8_stream_length(&ctx, p, len, 1, 1, NULL);

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).

Checklist

Before creating a PR, run through this checklist and mark each as complete:

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes

Ensuring that buf is always initialized in njs_throw_error_va()
and njs_error_fmt_new(), by requiring fmt to always be non NULL.

This fixes GCC warnings like:
169 | njs_unicode_decode_t ctx;
| ^
In function ‘njs_utf8_length’,
   inlined from ‘njs_error_new’ at src/njs_error.c:39:14,
   inlined from ‘njs_throw_error_va’ at src/njs_error.c:69:5:
   src/njs_utf8.h:141:12: error: ‘buf’ may be used uninitialized
   [-Werror=maybe-uninitialized]
   141 | return njs_utf8_stream_length(&ctx, p, len, 1, 1, NULL);
@xeioex xeioex requested a review from VadimZhestikov June 22, 2024 00:24
Copy link
Contributor

@VadimZhestikov VadimZhestikov left a comment

Choose a reason for hiding this comment

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

Looks good, approved, The only small thought is about error messages:

Below collected Error messages used by different engines. In this cases chrome provides bit more info to simplify debugging.
May be it would be nice behave like chrome?

String.fromCodePoint('_')
njs: RangeError: invalid code point
qjs: RangeError: invalid code point
node: RangeError: Invalid code point NaN
chrome: RangeError: Invalid code point NaN
firefox: RangeError: NaN is not a valid code point

'abc'.repeat(Infinity)
njs: RangeError: invalid count value
qjs: RangeError: invalid repeat count
node: RangeError: Invalid count value
chrome: RangeError: Invalid count value: Infinity
firefox: RangeError: repeat count must be less than infinity and not overflow maximum string size

''.repeat(-1)
njs: RangeError: invalid count value
qjs: RangeError: invalid repeat count
node: RangeError: Invalid count value
chrome: RangeError: Invalid count value: -1
FireFox: RangeError: repeat count must be non-negative

@xeioex
Copy link
Contributor Author

xeioex commented Jun 29, 2024

@VadimZhestikov

node: RangeError: Invalid code point NaN

I thought about it, but it requires calling of toString() for a invalid value just for an exception, not worth the effort I think.

@xeioex xeioex merged commit ed631d2 into nginx:master Jun 29, 2024
1 check passed
@xeioex xeioex deleted the maybe_uninit branch June 29, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants