Skip to content

Conversation

@rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Jan 22, 2026

This PR adds the stack-protector-strong flag to the library, which adds stack canaries to the Linux builds in order to mitigate several BinSkim issues.

TODO:

  • Confirm that performance does not regress.
  • Confirm that all files that need the flag have the flag.

@rzhao271 rzhao271 self-assigned this Jan 22, 2026
@rzhao271 rzhao271 added this to the January 2026 milestone Jan 22, 2026
@rzhao271 rzhao271 requested review from Tyriar and deepak1556 January 22, 2026 00:42
@rzhao271
Copy link
Contributor Author

Here's a quick test I tried adding to verify the performance. It did not regress when I switched from compiling with -fno-stack-protector to -fstack-protector-strong. In both cases, the recorded time was below 200ms after a few runs. Let me know whether the test needs work or whether I can add it to the PR.

describe('write performance', function () {
    it('writes 1000 characters within 2s', function (done) {
      this.timeout(10000);
      const term = new terminalConstructor(SHELL, [], {
        cols: 80,
        rows: 24,
        cwd: process.cwd(),
        env: process.env
      });
      const payloadBody = 'a'.repeat(995);
      const payload = 'echo ' + payloadBody + '\r';
      let buffer = '';
      const start = process.hrtime.bigint();
      term.on('data', function (data: string) {
        buffer += data;
        if (buffer.indexOf(payloadBody) !== -1) {
          term.kill();
        }
      });
      term.on('exit', function () {
        const durationMs = Number(process.hrtime.bigint() - start) / 1e6;
        console.log('writing 1000 characters took ' + durationMs.toFixed(2) + 'ms');
        assert.ok(
          durationMs < 2000,
          'writing 1000 characters took ' + durationMs.toFixed(2) + 'ms'
        );
        return done();
      });
      for (let i = 0; i < payload.length; i++) {
        term._write(payload[i]);
      }
    });
  });

@rzhao271 rzhao271 marked this pull request as ready for review January 22, 2026 00:58
Tyriar
Tyriar previously approved these changes Jan 22, 2026
deepak1556
deepak1556 previously approved these changes Jan 22, 2026
Copy link
Contributor

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks!

@rzhao271 rzhao271 dismissed stale reviews from deepak1556 and Tyriar via 1472d8a January 22, 2026 19:06
@rzhao271 rzhao271 enabled auto-merge (squash) January 22, 2026 19:10
@rzhao271 rzhao271 merged commit c69dc67 into main Jan 22, 2026
9 checks passed
@rzhao271 rzhao271 deleted the rzhao271/fsp branch January 22, 2026 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants