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

posix4e · 2016-12-01T23:27:40Z

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);

bnoordhuis · 2016-12-01T23:34:14Z

src/node_crypto.cc

+# endif
+# ifdef THIRTY_TWO_BIT
+        BIO_printf(bio, "0x%dx", exponent_word);
+# endif


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]

posix4e · 2016-12-02T02:39:53Z

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

bnoordhuis · 2016-12-02T09:47:08Z

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/

posix4e · 2016-12-02T17:55:14Z

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

Fishrock123 · 2016-12-02T20:32:07Z

Windows passed?

posix4e · 2016-12-03T00:26:46Z

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

gibfahn · 2016-12-03T01:41:03Z

@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/

posix4e · 2016-12-03T02:11:50Z

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?

posix4e · 2016-12-03T16:22:05Z

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

bnoordhuis · 2016-12-05T12:33:36Z

src/node_crypto.cc

      BIO_printf(bio, "0x%lx", exponent_word);
-
+# endif
+# ifdef THIRTY_TWO_BIT


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

bnoordhuis · 2016-12-05T12:41:05Z

src/node_crypto.cc

+# endif
+# ifdef THIRTY_TWO_BIT
+      BIO_printf(bio, "0x%dx", exponent_word);
+#endif


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

posix4e · 2016-12-15T00:59:05Z

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

MylesBorins · 2016-12-15T01:04:22Z

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

posix4e · 2016-12-15T01:55:51Z

Does this mean great build success?

MylesBorins · 2016-12-15T03:28:02Z

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);

posix4e · 2016-12-15T03:59:31Z

updated commit comment

MylesBorins · 2016-12-16T18:05:33Z

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

MylesBorins · 2016-12-16T19:34:51Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

posix4e commented Dec 1, 2016

bnoordhuis Dec 1, 2016

posix4e Dec 1, 2016

posix4e Dec 1, 2016

posix4e Dec 1, 2016 •

edited

Loading

posix4e Dec 1, 2016

posix4e commented Dec 2, 2016

bnoordhuis commented Dec 2, 2016

posix4e commented Dec 2, 2016

Fishrock123 commented Dec 2, 2016

posix4e commented Dec 3, 2016

gibfahn commented Dec 3, 2016

posix4e commented Dec 3, 2016

posix4e commented Dec 3, 2016

bnoordhuis Dec 5, 2016

posix4e Dec 5, 2016

bnoordhuis Dec 5, 2016

posix4e Dec 5, 2016 •

edited

Loading

posix4e commented Dec 15, 2016

MylesBorins commented Dec 15, 2016

posix4e commented Dec 15, 2016

MylesBorins commented Dec 15, 2016

posix4e commented Dec 15, 2016

MylesBorins commented Dec 16, 2016

MylesBorins commented Dec 16, 2016

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

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

Conversation

posix4e commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

bnoordhuis Dec 1, 2016

Choose a reason for hiding this comment

posix4e Dec 1, 2016

Choose a reason for hiding this comment

posix4e Dec 1, 2016

Choose a reason for hiding this comment

posix4e Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

posix4e Dec 1, 2016

Choose a reason for hiding this comment

posix4e commented Dec 2, 2016

bnoordhuis commented Dec 2, 2016

posix4e commented Dec 2, 2016

Fishrock123 commented Dec 2, 2016

posix4e commented Dec 3, 2016

gibfahn commented Dec 3, 2016

posix4e commented Dec 3, 2016

posix4e commented Dec 3, 2016

bnoordhuis Dec 5, 2016

Choose a reason for hiding this comment

posix4e Dec 5, 2016

Choose a reason for hiding this comment

bnoordhuis Dec 5, 2016

Choose a reason for hiding this comment

posix4e Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

posix4e commented Dec 15, 2016

MylesBorins commented Dec 15, 2016

posix4e commented Dec 15, 2016

MylesBorins commented Dec 15, 2016

posix4e commented Dec 15, 2016

MylesBorins commented Dec 16, 2016

MylesBorins commented Dec 16, 2016

posix4e Dec 1, 2016 •

edited

Loading

posix4e Dec 5, 2016 •

edited

Loading