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

cares: Support malloc(0) scenarios for AIX #6305

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
8 participants
@gireeshpunathil
Copy link
Member

commented Apr 20, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

@mscdex mscdex added dns build labels Apr 20, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LGTM

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Does that mean that c-ares often calls malloc(0)? Shouldn't that also be fixed upstream?
These options may work for aix, but I'm not sure that this doesn't affect other systems.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

It may very well need to be. In this particular case, the options are only being set for aix so there shouldn't be any impact on other systems for this particular change.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

@jasnell That's exactly the problem here. This patch only fixes this for aix, and using a vendor-specific hack.

malloc(0) shouldn't be used, and if c-ares calls that — it's a problem by itself, that could affect more platforms than just aix.

Note: I'm not against this patch, as it should solve the issue for aix. But perhaps this should be also propertly fixed later.

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

I believe it is a question of programming style. Whether you want the size check before calling malloc, or you want to go ahead and allocate, but skip further processing of data eventually, as there was no demand for memory in the first place.

The problem source here is

hostent->h_addr_list[0] = ares_malloc(addrlen);

If you validate request size before malloc call, I believe there are quite many places we will need that change. For example this has been the case for v8 for a while, and the workaround was to keep the source in-tact, and address AIX case specifically. See also https://github.com/nodejs/node/blob/master/deps/v8/build/toolchain.gypi#L1086

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

A better fix IMO is to change the call to ares_library_init() in src/cares_wrap.cc to ares_library_init_mem() and pass in malloc/realloc wrappers that change zero-sized allocations to one-sized allocations.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

On second thought, I can live with the _LINUX_SOURCE_COMPAT hack if V8 is doing that as well. LGTM.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LTGM

@mhdawson mhdawson self-assigned this Apr 20, 2016

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Given the LGTMs I'll plan to land Thursday unless there are objections before then.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

CI clean except for infrastructure related failure on windows so going to land.

mhdawson added a commit that referenced this pull request Apr 21, 2016

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

PR-URL: #6305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Landed as 40ede46

@mhdawson mhdawson closed this Apr 21, 2016

@jbergstroem

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

Did this go upstream? A bit quick to land imo, especially seeing how we're trying to improve the shared library state.

@jbergstroem

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

Hm, I'll check if the current c-ares build system does this already. If so, pardon my negligence.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

cares.gyp is custom to our build, it doesn't exist upstream.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

@jbergstroem The correct way of fixing this upstream would be making it not call malloc(0).

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

PR-URL: nodejs#6305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

PR-URL: #6305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

@mhdawson is this something we want for LTS?

@mhdawson

This comment has been minimized.

Copy link
Member

commented Jun 2, 2016

@thealphanerd yes it should be safe and would be good to have in LTS

MylesBorins added a commit that referenced this pull request Jun 2, 2016

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

PR-URL: #6305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>

@ChALkeR ChALkeR referenced this pull request Jun 14, 2016

Closed

src: temporary fix for AIX malloc issue #7229

2 of 2 tasks complete

@MylesBorins MylesBorins referenced this pull request Jun 24, 2016

Merged

v4.4.7 proposal #7395

MylesBorins added a commit that referenced this pull request Jun 24, 2016

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

PR-URL: #6305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Jun 24, 2016

cares: Support malloc(0) scenarios for AIX
Many places in cares library, when the stream data arrives
from the network with respect to dns and reverse dns
resolution, they are populated into data structures created
dymaically based on the size of the data. Malloc is heavily
used for such cases.

Often, based on the data length, malloc(0) is invoked. Linux
behavior on zero byte allocation is to return a valid pointer
where in AIX, it always return NULL.

This manifestst as test failure of test/internet/test-dns.js

Solution is to build cares with Linux compatible malloc behavior

PR-URL: #6305
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.