src: fix string format mistake for 32 bit node by posix4e · Pull Request #10082 · nodejs/node · GitHub

Checklist

make -j8 test

Affected core subsystem(s)

crypto

Description of change

I noticed a warning reminscint of a format-string vuln. I think i have made the format string correct on non alpha based systems.
The warning was

  unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=]
     BIO_printf(bio, "0x%lx", exponent_word);

A less ifdef-y approach would be BIO_printf(bio, "0x%llx", static_cast<uint64_t>(exponent_word));

I get

foo.c:16:65: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
BIO_printf(bio, "0x%llx", static_cast<uint64_t>(exponent_word));

When I try 64bit mode

@bnoordhuis

BIO_printf(bio, "0x%llx", static_cast<long long unsigned>(exponent_word));
works though

Oh damn but then I get

src/node_crypto.cc:1540: Use int16/int64/etc, rather than the C type long [runtime/int] [4]

I think this gets us the best of both worlds @bnoordhuis ?

I'm fairly sure Visual Studio doesn't have inttypes.h but we'll find out soon enough: https://ci.nodejs.org/job/node-test-pull-request/5099/

Looks like you were right? Shall we go back to ifdefs ?

Windows passed?

Looks like nothing passed. Maybe I should try the #ifdef approach again?

@posix4e Lots of other stuff failed, but it looks like windows did indeed pass.

https://ci.nodejs.org/job/node-test-commit-windows-fanned/5557/

Forgive my noobness but what are the next steps? Shall I return the ifdefs or is this sufficient? Does anyone agree about the merit of the patch?It seems windows might have passed (again excuse my noob) what is causing the other build failures?

Ok going back to my original ifdef patch unless anyone has some suggestions?

Can this be an #else?

No, not exactly, there's a third case that I was hoping would break. I can handle it, it only happens on VMS and alpha

Thinking about it more, what about this?

uint64_t exponent_word = static_cast<uint64_t>(BN_get_word(rsa->e));
uint32_t lo = static_cast<uint32_t>(exponent_word);
uint32_t hi = static_cast<uint32_t>(exponent_word >> 32);
if (hi == 0) {
  BIO_printf(bio, "0x%x", lo);
} else {
  BIO_printf(bio, "0x%x%08x", hi, lo);
}

FIne by me. Seems less clear, but if you'd prefer

ring a ling a ding dong. Anything else we should do?

one more run of ci: https://ci.nodejs.org/job/node-test-pull-request/5410/

oh hi @posix4e o/

also can you modify the commit to include the subsystem and under 50 characters, might I suggest
src: fix string format mistake for 32 bit node

Does this mean great build success?

generally we don't put a colon at the end of the commit title. Otherwise the commit looks in order. I'll have to defer to @bnoordhuis for the actual review.

@addaleax may be able to help out as well.

warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=] BIO_printf(bio, "0x%lx", exponent_word);

updated commit comment

I'll land this end of day if there are no objections
/cc @bnoordhuis

landed in 40eba12

warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=] BIO_printf(bio, "0x%lx", exponent_word); PR-URL: #10082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * **crypto**: - Allow adding extra certificates to well-known CAs. (Sam Roberts) [#9139](#9139) * **buffer**: - Fix single-character string filling. (Anna Henningsen) [#9837](#9837) - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen) [#9837](#9837) * **url**: - Add inspect function to TupleOrigin. (Safia Abdalla) [#10039](#10039) - Including base argument in originFor. (joyeecheung) [#10021](#10021) - Improve URLSearchParams spec compliance. (Timothy Gu) [#9484](#9484) * **http**: - Remove stale timeout listeners. (Karl Böhlmark) [#9440](#9440) * **build**: - Fix node_g target. (Daniel Bevenius) [#10153](#10153) * **fs**: - Remove unused argument from copyObject(). (EthanArrowood) [#10041](#10041) * **timers**: - Fix handling of cleared immediates. (hveldstra) [#9759](#9759) * **src**: - Add wrapper for process.emitWarning(). (SamRoberts) [#9139](#9139) - Fix string format mistake for 32 bit node.(Alex Newman) [#10082](#10082)

warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=] BIO_printf(bio, "0x%lx", exponent_word); PR-URL: nodejs/node#10082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=] BIO_printf(bio, "0x%lx", exponent_word); PR-URL: #10082 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 1, 2016

bnoordhuis reviewed Dec 1, 2016

View reviewed changes

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 1, 2016

posix4e force-pushed the build-fix branch 4 times, most recently from c78cee8 to cca4236 Compare December 2, 2016 00:05

posix4e force-pushed the build-fix branch from cca4236 to 040b521 Compare December 3, 2016 16:21

bnoordhuis reviewed Dec 5, 2016

View reviewed changes

posix4e force-pushed the build-fix branch from 040b521 to 7b654f0 Compare December 5, 2016 19:13

posix4e force-pushed the build-fix branch from 7b654f0 to 09c0c92 Compare December 15, 2016 01:31

nodejs-github-bot added lts-watch-v6.x labels Dec 15, 2016

src: fix string format mistake for 32 bit node

47ca967

warning: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘unsigned int’ [-Wformat=] BIO_printf(bio, "0x%lx", exponent_word);

posix4e force-pushed the build-fix branch from 09c0c92 to 47ca967 Compare December 15, 2016 03:59

nodejs-github-bot added lts-watch-v6.x labels Dec 15, 2016

posix4e changed the title On 32 bit node we have a string format mistake: src: fix string format mistake for 32 bit node Dec 15, 2016

addaleax approved these changes Dec 15, 2016

View reviewed changes

MylesBorins self-assigned this Dec 16, 2016

bnoordhuis approved these changes Dec 16, 2016

View reviewed changes

MylesBorins closed this Dec 16, 2016

italoacasas mentioned this pull request Dec 17, 2016

v7.3.0 proposal #10277

Merged

posix4e mentioned this pull request Dec 20, 2016

src: fix string format mistake for 32 bit node brave/node#2

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

This was referenced Jan 24, 2017

v4.8.0 proposal #10973

Merged

v6.10.0 proposal #10974

Merged

src: fix string format mistake for 32 bit node by posix4e · Pull Request #10082 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix string format mistake for 32 bit node - #10082

src: fix string format mistake for 32 bit node#10082posix4e wants to merge 1 commit intonodejs:masternodejs/node:masterfrom posix4e:build-fixposix4e/node:build-fixCopy head branch name to clipboard

Conversation

posix4e commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

posix4e Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

posix4e Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

posix4e Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

posix4e Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

posix4e commented Dec 2, 2016

Uh oh!

bnoordhuis commented Dec 2, 2016

Uh oh!

posix4e commented Dec 2, 2016

Uh oh!

Fishrock123 commented Dec 2, 2016

Uh oh!

posix4e commented Dec 3, 2016

Uh oh!

gibfahn commented Dec 3, 2016

Uh oh!

posix4e commented Dec 3, 2016

Uh oh!

posix4e commented Dec 3, 2016

Uh oh!

bnoordhuis Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

posix4e Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

posix4e Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

posix4e commented Dec 15, 2016

Uh oh!

MylesBorins commented Dec 15, 2016

Uh oh!

posix4e commented Dec 15, 2016

Uh oh!

MylesBorins commented Dec 15, 2016

Uh oh!

posix4e commented Dec 15, 2016

Uh oh!

MylesBorins commented Dec 16, 2016

Uh oh!

MylesBorins commented Dec 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: fix string format mistake for 32 bit node#10082
posix4e wants to merge 1 commit into
nodejs:masterfrom
posix4e:build-fix

posix4e Dec 1, 2016 •
edited
Loading

posix4e Dec 5, 2016 •
edited
Loading