Skip to content

Commit 9339d69

Browse files
committed
Fix bugs in rosidl_gen and index.js, add tests (#1412)
This PR fixes a couple of correctness issues in message generation and generator version detection, and adds targeted tests to prevent regressions in `rosidl_gen` primitive type initialization. **Changes:** **index.js** - Fix file descriptor leak in `getCurrentGeneratorVersion()`: replaced `fs.open()` followed by nested `fs.readFile()` with a single `fs.readFile()` call, eliminating the unclosed fd when the file exists - Add `try/catch` around `JSON.parse()` to properly reject on malformed JSON instead of throwing an unhandled exception **rosidl_gen/primitive_types.js** - Fix operator precedence bug in `initString()`: `(!str) instanceof Buffer` always evaluated to `false` (negating `str` first, then checking if `true`/`false` is an instance of `Buffer`); corrected to `!(str instanceof Buffer)` so the type guard works as intended - Same fix for the `(!str) instanceof StringRefStruct` check in the `own=false` branch **rosidl_gen/packages.js** - Fix TOCTOU race in `generateMsgForSrv()`: replaced `existsSync()` + `mkdirSync()` with `mkdirSync(path, { recursive: true })`, which is atomic and won't throw if the directory already exists **test/test-primitive-types.js** *(new)* - Add 9 unit tests for `initString()` covering both `own=true` and `own=false` branches: rejects invalid types (number, string, null, undefined, Buffer-when-struct-expected) and correctly initializes a valid `StringRefStruct` Fix: #1411
1 parent 08f9e0f commit 9339d69

4 files changed

Lines changed: 104 additions & 13 deletions

File tree

index.js

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -85,21 +85,19 @@ async function getCurrentGeneratorVersion() {
8585
const jsonFilePath = path.join(generator.generatedRoot, 'generator.json');
8686

8787
return new Promise((resolve, reject) => {
88-
fs.open(jsonFilePath, 'r', (err) => {
88+
fs.readFile(jsonFilePath, 'utf8', (err, data) => {
8989
if (err) {
9090
if (err.code === 'ENOENT') {
9191
resolve(null);
9292
} else {
9393
reject(err);
9494
}
9595
} else {
96-
fs.readFile(jsonFilePath, 'utf8', (err, data) => {
97-
if (err) {
98-
reject(err);
99-
} else {
100-
resolve(JSON.parse(data).version);
101-
}
102-
});
96+
try {
97+
resolve(JSON.parse(data).version);
98+
} catch (parseErr) {
99+
reject(parseErr);
100+
}
103101
}
104102
});
105103
});

rosidl_gen/packages.js

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,7 @@ async function generateMsgForSrv(filePath, interfaceInfo, pkgMap) {
150150
const arr = data.split(/-{3,}/);
151151
if (arr.length == 2) {
152152
const packagePath = path.join(serviceMsgPath, interfaceInfo.pkgName);
153-
if (!fs.existsSync(packagePath)) {
154-
fs.mkdirSync(packagePath);
155-
}
153+
fs.mkdirSync(packagePath, { recursive: true });
156154

157155
await fsp.writeFile(path.join(packagePath, requestMsgName), arr[0]);
158156
await fsp.writeFile(path.join(packagePath, responseMsgName), arr[1]);

rosidl_gen/primitive_types.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,14 @@ const StringRefStruct = StructType({
2626

2727
function initString(str, own = false) {
2828
if (own) {
29-
if ((!str) instanceof Buffer) {
29+
if (!(str instanceof Buffer)) {
3030
throw new TypeError(
3131
'Invalid argument: should provide a Node Buffer to bindingsStringInit()'
3232
);
3333
}
3434
rclnodejs.initString(str);
3535
} else {
36-
if ((!str) instanceof StringRefStruct) {
36+
if (!(str instanceof StringRefStruct)) {
3737
throw new TypeError(
3838
'Invalid argument: should provide a type of StringRefStruct'
3939
);

test/test-primitive-types.js

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
// Copyright (c) 2026, The Robot Web Tools Contributors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
'use strict';
16+
17+
const assert = require('assert');
18+
const primitiveTypes = require('../rosidl_gen/primitive_types.js');
19+
20+
describe('rosidl_gen primitive_types', function () {
21+
describe('initString(str, own=true)', function () {
22+
it('should throw TypeError when str is a number', function () {
23+
assert.throws(
24+
() => primitiveTypes.initString(123, true),
25+
TypeError,
26+
'should provide a Node Buffer'
27+
);
28+
});
29+
30+
it('should throw TypeError when str is a plain string', function () {
31+
assert.throws(
32+
() => primitiveTypes.initString('hello', true),
33+
TypeError,
34+
'should provide a Node Buffer'
35+
);
36+
});
37+
38+
it('should throw TypeError when str is null', function () {
39+
assert.throws(
40+
() => primitiveTypes.initString(null, true),
41+
TypeError,
42+
'should provide a Node Buffer'
43+
);
44+
});
45+
46+
it('should throw TypeError when str is undefined', function () {
47+
assert.throws(
48+
() => primitiveTypes.initString(undefined, true),
49+
TypeError,
50+
'should provide a Node Buffer'
51+
);
52+
});
53+
});
54+
55+
describe('initString(str, own=false)', function () {
56+
it('should throw TypeError when str is a number', function () {
57+
assert.throws(
58+
() => primitiveTypes.initString(123),
59+
TypeError,
60+
'should provide a type of StringRefStruct'
61+
);
62+
});
63+
64+
it('should throw TypeError when str is a plain string', function () {
65+
assert.throws(
66+
() => primitiveTypes.initString('hello'),
67+
TypeError,
68+
'should provide a type of StringRefStruct'
69+
);
70+
});
71+
72+
it('should throw TypeError when str is a Buffer', function () {
73+
assert.throws(
74+
() => primitiveTypes.initString(Buffer.alloc(10)),
75+
TypeError,
76+
'should provide a type of StringRefStruct'
77+
);
78+
});
79+
80+
it('should throw TypeError when str is null', function () {
81+
assert.throws(
82+
() => primitiveTypes.initString(null),
83+
TypeError,
84+
'should provide a type of StringRefStruct'
85+
);
86+
});
87+
88+
it('should initialize a valid StringRefStruct', function () {
89+
const str = new primitiveTypes.string();
90+
primitiveTypes.initString(str);
91+
assert.strictEqual(str.size, 0);
92+
assert.strictEqual(str.capacity, 1);
93+
});
94+
});
95+
});

0 commit comments

Comments
 (0)