Skip to content
This repository has been archived by the owner on May 14, 2024. It is now read-only.

Fix crash in res.send() #686

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Fix crash in res.send() #686

merged 1 commit into from
Dec 3, 2020

Conversation

gramakri
Copy link
Contributor

@gramakri gramakri commented Dec 2, 2020

In commit c6fa259, "var" usage
was replaced with const. The code previously worked because "var"
has function scoping unlike const which has block scoping.

TypeError: Cannot read property 'attributes' of null

In commit c6fa259, "var" usage
was replaced with const. The code previously worked because "var"
has function scoping unlike const which has block scoping.

TypeError: Cannot read property 'attributes' of null
@gramakri
Copy link
Contributor Author

gramakri commented Dec 2, 2020

The backtrace we got is

2020-12-01T17:39:21.517Z box:ldap TypeError: Cannot read property 'attributes' of null
    at /home/yellowtent/box/node_modules/ldapjs/lib/messages/search_response.js:100:12
    at Array.forEach (<anonymous>)
    at SearchResponse.send (/home/yellowtent/box/node_modules/ldapjs/lib/messages/search_response.js:99:29)
    at /home/yellowtent/box/src/ldap.js:113:17
    at Array.forEach (<anonymous>)
    at finalSend (/home/yellowtent/box/src/ldap.js:112:17)
    at /home/yellowtent/box/src/ldap.js:370:17
    at /home/yellowtent/box/node_modules/async/dist/async.js:473:16
    at replenish (/home/yellowtent/box/node_modules/async/dist/async.js:1006:25)
    at iterateeCallback (/home/yellowtent/box/node_modules/async/dist/async.js:995:17) %s failure to write message %j 172.18.0.7:57498 { messageID: 1075,
  protocolOp: 'LDAPResult',
  status: 0,
  matchedDN: '',
  errorMessage: '',
  referrals: [],
  controls: [] }

@UziTech
Copy link
Member

UziTech commented Dec 3, 2020

I wonder if there are more cases of variable shadowing gone wrong. We should add a rule to the linter to prevent this.

@jsumners
Copy link
Member

jsumners commented Dec 3, 2020

This is what I was afraid of in #674.

Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

I really want to ask for test coverage here to prevent a regression, but I'm unclear how feasible that would be.

@jsumners jsumners merged commit 582cd2b into ldapjs:master Dec 3, 2020
@UziTech
Copy link
Member

UziTech commented Dec 3, 2020

I did a search with eslint and only found this shadowing to be a problem. I would prefer to move to eslint and use eslint-config-standard instead of using standard so we can add the no-shadow rule.

During that search I also found that the callback on getEntryChangeNotificationControl is never used. I'm not sure if that is a problem.

function getEntryChangeNotificationControl (req, obj, callback) {
// if we want to return a ECNC
if (req.persistentSearch.value.returnECs) {
const attrs = obj.attributes
const value = {}
value.changeType = getOperationType(attrs.changetype)
// if it's a modDN request, fill in the previous DN
if (value.changeType === 8 && attrs.previousDN) {
value.previousDN = attrs.previousDN
}
value.changeNumber = attrs.changenumber
return new EntryChangeNotificationControl({ value: value })
} else {
return false
}
}

@jsumners
Copy link
Member

jsumners commented Dec 3, 2020

I would prefer to move to eslint and use eslint-config-standard instead of using standard so we can add the no-shadow rule.

🤔 the point of using standard though is that we shouldn't have to keep playing the "update rules" game. We can discuss in a separate issue.

During that search I also found that the callback on getEntryChangeNotificationControl is never used. I'm not sure if that is a problem.

I don't know if it is, but I'd just leave it. One day someone will work up the constitution to clean this whole mess up and these things will get resolved.

@gramakri gramakri deleted the send_crash branch December 3, 2020 04:30
@jsumners
Copy link
Member

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.

@ldapjs ldapjs locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants