dns: report out of memory properly #20317

BridgeAR · 2018-04-26T03:05:29Z

This addresses a part of a TODO by properly reporting an out of
memory error to the user instead of reporting ENOTFOUND.

I only changed this single entry as this is super rare and should be safe to change.
Since it is a OOM, we also have no test for it.

The other two are likely used a lot and a lot of userland code will rely on that.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2018-04-26T03:05:57Z

CI https://ci.nodejs.org/job/node-test-pull-request/14512/

Trott · 2018-04-26T03:41:25Z

Since it is a OOM, we also have no test for it.

I added a test to your branch. It's in its own commit so you can remove that commit if you're 👎 on the test, but I think it's probably useful.

bnoordhuis · 2018-04-26T07:31:18Z

lib/internal/errors.js

@@ -639,8 +638,7 @@ function dnsException(code, syscall, hostname) {
  if (typeof code === 'number') {
    // FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass
    // the true error to the user. ENOTFOUND is not even a proper POSIX error!
-    if (code === UV_EAI_MEMORY ||
-        code === UV_EAI_NODATA ||
+    if (code === UV_EAI_NODATA ||
        code === UV_EAI_NONAME) {


Fits on one line now.

This addresses a part of a TODO by properly reporting an out of memory error to the user instead of reporting `ENOTFOUND`.

Add test that confirms that when libuv reports a memory error on a DNS query, that the memory error is passed through and not replaced with ENOTFOUND.

BridgeAR · 2018-04-26T10:49:54Z

Nit addressed. New CI: https://ci.nodejs.org/job/node-test-pull-request/14520/

@Trott I guess it is best to land this as separate commits and not squash?

BridgeAR · 2018-04-26T10:50:24Z

Ah, I fixed the commit message @Trott (typo in memory).

Trott · 2018-04-26T21:32:31Z

@Trott I guess it is best to land this as separate commits and not squash?

@BridgeAR That's what I'd do, but either way is fine by me.

Trott · 2018-04-26T21:34:32Z

Hmmm...puzzling. A relevant test fails but only SmartOS.

Trott · 2018-04-26T21:35:59Z

Re-running SmartOS just in case it was some kind of weird quirk: https://ci.nodejs.org/job/node-test-commit-smartos/17213/

Trott · 2018-04-26T22:12:11Z

So, test-http-dns-error (which is not the test added in this PR) fails on SmartOS with this code change because an error that used to be ENOTFOUND is now showing up as EAI_MEMORY on SmartOS only. Paging @nodejs/platform-smartos for advice/debugging/something.

BridgeAR · 2018-04-27T11:33:53Z

@bnoordhuis could you have a look at the test failure? You added it originally.

I personally can not see anything special or wrong in the test, so it seems to be a low level platform specific issue.

/cc @nodejs/libuv

bnoordhuis · 2018-04-27T11:42:29Z

Not much to look at. The system resolver reports EAI_MEMORY for the invalid hostname. I guess the logic here is that '*'.repeat(256) is longer than what DNS allows because that's only 255 bytes.

BridgeAR · 2018-04-27T12:32:07Z

@bnoordhuis thanks for clariying. But is UV_EAI_MEMORY really the best error in such a case? And do you know why it only fails on SmartOS?

BridgeAR · 2018-04-27T12:34:46Z

SmartOS CI: https://ci.nodejs.org/job/node-test-commit-smartos/17226/

BridgeAR · 2018-04-27T12:54:26Z

@bnoordhuis reducing the hostname to a byteLength of 250 still returns the OOM error.

08:45:42     AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
08:45:42     + expected - actual
08:45:42     
08:45:42     - 'EAI_MEMORY'
08:45:42     + 'ENOTFOUND'

Trott · 2018-04-27T22:53:48Z

test/parallel/test-http-dns-error.js

@@ -29,7 +29,7 @@ const assert = require('assert');
 const http = require('http');
 const https = require('https');

-const host = '*'.repeat(256);
+const host = '*'.repeat(250);


I know you were doing this just to see if the OOM error went away on SmartOS, but I think 256 was chosen specifically because 255 is the maximum length for a hostname. I believe the idea here is to trigger a hostname-too-long error specifically. So this change should not land, I suspect.

Trott · 2018-04-27T23:25:02Z

Here's a simple test case:

const cares = process.binding('cares_wrap');
const {
  UV_EAI_MEMORY,
  UV_EAI_NODATA,
  UV_EAI_NONAME
} = process.binding('uv');

var req = new cares.GetAddrInfoReqWrap();
req.family = 0;
req.hostname = '*'.repeat(256);
req.oncomplete = (e) => { 
 console.log(`error code was ${e}`);
 console.log(`UV_EAI_MEMORY: ${UV_EAI_MEMORY}`);
 console.log(`UV_EAI_NODATA: ${UV_EAI_NODATA}`);
 console.log(`UV_EAI_NONAME: ${UV_EAI_NONAME}`);
}

var err = cares.getaddrinfo(req, req.hostname, req.family, 1024, false);

This straightforward test case does not result in UV_EAI_MEMORY on the SmartOS hosts in CI:

[root@0410827e-87c7-449e-b16c-d626f9743aa6 /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64]# /var/tmp/node /var/tmp/test.js 
error code was -3002
UV_EAI_MEMORY: -3006
UV_EAI_NODATA: -3007
UV_EAI_NONAME: -3008
[root@0410827e-87c7-449e-b16c-d626f9743aa6 /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos16-64]#

So something more....subtle...is going on with the SmartOS hosts in CI.

Trott

Defensively giving this a red X while the 256 is down to 250 because I do think that may invalidate the intended test case.

Trott · 2018-04-27T23:26:51Z

Oh, wait, -3002 is UV_EAI_BADFLAGS so there's something wrong with my "simple test case" code above...

Trott · 2018-04-27T23:40:44Z

OK, here's code to replicate the problem:

const cares = process.binding('cares_wrap');
const {
  UV_EAI_MEMORY,
  UV_EAI_NODATA,
  UV_EAI_NONAME
} = process.binding('uv');

var req = new cares.GetAddrInfoReqWrap();
req.family = 0;
req.hostname = '*'.repeat(256);
req.oncomplete = (e) => { 
 console.log(`error code was ${e}`);
 console.log(`UV_EAI_MEMORY: ${UV_EAI_MEMORY}`);
 console.log(`UV_EAI_NODATA: ${UV_EAI_NODATA}`);
 console.log(`UV_EAI_NONAME: ${UV_EAI_NONAME}`);
}

cares.getaddrinfo(req, req.hostname, req.family, 0, false);

Running this on SmartOS:

error code was -3006
UV_EAI_MEMORY: -3006
UV_EAI_NODATA: -3007
UV_EAI_NONAME: -3008

UV_EAI_MEMORY? That's no good.

For comparison, running it locally on my Macbook results -3008 which is UV_EAI_NONAME which is what is expected.

Back to SmartOS:

Changing '*'.repeat(256) to 'a'.repeat(256) does not change the result.
Changing '*'.repeat(256) to '*'.repeat(255) does change the result to UV_EAI_NONAME.

So there's something specific about length of 256 that results in an OOM error but I have to imagine that OOM error is spurious. (And I don't know why @BridgeAR got different results when changing the hostname length in the test to 250.)

BridgeAR · 2018-04-27T23:55:41Z

@Trott I did not plan on landing it with the test change. I am not really sure what to do with your findings though due to the difference with my change...

Trott · 2018-04-28T00:00:24Z

@BridgeAR It seems like this is probably either a SmartOS quirk or else a libuv bug on SmartOS. I'm not really sure how to debug. The intersection of SmartOS and libuv would seem to be @cjihrig so maybe we can try to get their input... But while we're at it: @nodejs/platform-smartos @nodejs/libuv

Trott · 2018-04-28T00:01:08Z

Oh, for newly-looped-in people who want the TL;DR, I think the crux of the problem is described in #20317 (comment).

Trott · 2018-04-28T00:01:26Z

(I suppose it could also be a cares bug?)

bnoordhuis · 2018-04-28T19:07:15Z

Libuv calls getaddrinfo(), the system resolver. It's a smartos quirk / bug.

Trott · 2018-04-28T20:21:08Z

Libuv calls getaddrinfo(), the system resolver. It's a smartos quirk / bug.

I bet this SmartOS behavior is probably why EAI_MEMORY was mapped to ENOTFOUND in the first place!

Trott · 2018-04-28T20:35:35Z

Sure enough this C program confirms it's specific to SmartOS (or at least our SmartOS hosts in CI):

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>
struct addrinfo hints, *infoptr; // So no need to use memset global variables

int main() {
  hints.ai_family = AF_INET; // AF_INET means IPv4 only addresses

  int result = getaddrinfo("****************************************************************************************************************************************************************************************************************************************************************", NULL, &hints, &infoptr);
  if (result) {
    fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(result));
    exit(1);
  }

  struct addrinfo *p;
  char host[256];

  for(p = infoptr; p != NULL; p = p->ai_next) {

    getnameinfo(p->ai_addr, p->ai_addrlen, host, sizeof(host), NULL, 0,
NI_NUMERICHOST);
    puts(host);
  }

  freeaddrinfo(infoptr);
  return 0;
}

(This is trivially modified from sample code at https://github.com/angrave/SystemProgramming/wiki/Networking,-Part-2:-Using-getaddrinfo.)

On SmartOS, I put it in test.c and compiled it with cc -lnsl -lsocket -lresolv test.c.

[root@527b0138-1d1d-6bcc-88a3-9968feaab1eb /var/tmp]# ./a.out
getaddrinfo: memory allocation failure
[root@527b0138-1d1d-6bcc-88a3-9968feaab1eb /var/tmp]#

For comparison, when running on macOS:

$ ./a.out
getaddrinfo: nodename nor servname provided, or not known
$

Trott · 2018-04-28T20:51:27Z

Looking through git history, I'm unconvinced of the correctness of my recollection that 256 length is significant. 64 is long enough to guarantee the hostname is invalid as long as there's no . in it. And at that length, we don't get a memory error on SmartOS. Want to try 64, @BridgeAR?

Trott · 2018-04-28T20:52:22Z

(If you change it to 64 and the test starts passing, feel free to dismiss my request for changes.)

BridgeAR · 2018-04-29T14:39:24Z

CI https://ci.nodejs.org/job/node-test-pull-request/14582/

BridgeAR · 2018-04-29T17:32:43Z

~~I changed the wrong test...~~
I missed one test that has to be fixed as well.

BridgeAR · 2018-04-29T17:35:10Z

CI https://ci.nodejs.org/job/node-test-pull-request/14589/

tests are green

This addresses a part of a TODO by properly reporting an out of memory error to the user instead of reporting `ENOTFOUND`. PR-URL: nodejs#20317 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Add test that confirms that when libuv reports a memory error on a DNS query, that the memory error is passed through and not replaced with ENOTFOUND. PR-URL: nodejs#20317 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

BridgeAR · 2018-04-30T14:51:02Z

Landed in a158d41 and 0deb27b

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 26, 2018

BridgeAR requested a review from bnoordhuis April 26, 2018 03:05

nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Apr 26, 2018

BridgeAR requested a review from a team April 26, 2018 03:05

bnoordhuis approved these changes Apr 26, 2018

View reviewed changes

BridgeAR and others added 2 commits April 26, 2018 12:48

dns: report out of memory properly

012d5ae

This addresses a part of a TODO by properly reporting an out of memory error to the user instead of reporting `ENOTFOUND`.

test: add dns memory error test

1609ba1

Add test that confirms that when libuv reports a memory error on a DNS query, that the memory error is passed through and not replaced with ENOTFOUND.

BridgeAR force-pushed the dns-properly-report-oom branch from b91f209 to 1609ba1 Compare April 26, 2018 10:48

cjihrig approved these changes Apr 26, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2018

trivikr approved these changes Apr 26, 2018

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2018

fixup: try fix

d91041c

Trott reviewed Apr 27, 2018

View reviewed changes

Trott previously requested changes Apr 27, 2018

View reviewed changes

fixup: reduce hostname length to 64 chars

d80f127

fixup

b6cba50

BridgeAR closed this Apr 30, 2018

BridgeAR deleted the dns-properly-report-oom branch January 20, 2020 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: report out of memory properly #20317

dns: report out of memory properly #20317

BridgeAR commented Apr 26, 2018

BridgeAR commented Apr 26, 2018

Trott commented Apr 26, 2018

bnoordhuis Apr 26, 2018

BridgeAR commented Apr 26, 2018

BridgeAR commented Apr 26, 2018

Trott commented Apr 26, 2018

Trott commented Apr 26, 2018 •

edited

Loading

Trott commented Apr 26, 2018

Trott commented Apr 26, 2018 •

edited

Loading

BridgeAR commented Apr 27, 2018

bnoordhuis commented Apr 27, 2018

BridgeAR commented Apr 27, 2018

BridgeAR commented Apr 27, 2018

BridgeAR commented Apr 27, 2018

Trott Apr 27, 2018

Trott commented Apr 27, 2018

Trott left a comment

Trott commented Apr 27, 2018

Trott commented Apr 27, 2018 •

edited

Loading

BridgeAR commented Apr 27, 2018

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018

bnoordhuis commented Apr 28, 2018

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018 •

edited

Loading

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018

BridgeAR commented Apr 29, 2018

BridgeAR commented Apr 29, 2018 •

edited

Loading

BridgeAR commented Apr 29, 2018

BridgeAR commented Apr 30, 2018

dns: report out of memory properly #20317

dns: report out of memory properly #20317

Conversation

BridgeAR commented Apr 26, 2018

Checklist

BridgeAR commented Apr 26, 2018

Trott commented Apr 26, 2018

bnoordhuis Apr 26, 2018

Choose a reason for hiding this comment

BridgeAR commented Apr 26, 2018

BridgeAR commented Apr 26, 2018

Trott commented Apr 26, 2018

Trott commented Apr 26, 2018 • edited Loading

Trott commented Apr 26, 2018

Trott commented Apr 26, 2018 • edited Loading

BridgeAR commented Apr 27, 2018

bnoordhuis commented Apr 27, 2018

BridgeAR commented Apr 27, 2018

BridgeAR commented Apr 27, 2018

BridgeAR commented Apr 27, 2018

Trott Apr 27, 2018

Choose a reason for hiding this comment

Trott commented Apr 27, 2018

Trott left a comment

Choose a reason for hiding this comment

Trott commented Apr 27, 2018

Trott commented Apr 27, 2018 • edited Loading

BridgeAR commented Apr 27, 2018

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018

bnoordhuis commented Apr 28, 2018

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018 • edited Loading

Trott commented Apr 28, 2018

Trott commented Apr 28, 2018

BridgeAR commented Apr 29, 2018

BridgeAR commented Apr 29, 2018 • edited Loading

BridgeAR commented Apr 29, 2018

BridgeAR commented Apr 30, 2018

Trott commented Apr 26, 2018 •

edited

Loading

Trott commented Apr 26, 2018 •

edited

Loading

Trott commented Apr 27, 2018 •

edited

Loading

Trott commented Apr 28, 2018 •

edited

Loading

BridgeAR commented Apr 29, 2018 •

edited

Loading