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

%.* is not a supported debug specifier #35688

Closed
mmomtchev opened this issue Oct 16, 2020 · 3 comments
Closed

%.* is not a supported debug specifier #35688

mmomtchev opened this issue Oct 16, 2020 · 3 comments
Labels
http2 Issues or PRs related to the http2 subsystem. inspector Issues and PRs related to the V8 inspector protocol

Comments

@mmomtchev
Copy link
Contributor

  • Version: v14.13.1
  • Platform: Linux
  • Subsystem: http2

What steps will reproduce the bug?

(example from #35475)
NODE_DEBUG_NATIVE=HTTP2SESSION node

const http2 = require('http2');
const tls = require('tls');
const stream = require('stream');

const options = {
    ALPNProtocols: ['h2'],
    host: 'nghttp2.org',
    servername: 'nghttp2.org',
    port: 443
};

const socket = tls.connect(options, async () => {
    console.log('TLS Connected!');

    const proxy = new stream.Duplex({
        read(...args) {
            const x = socket.read();
            console.log('read', JSON.stringify(x));
            if (x !== null)
            this.push(x);
        },

        final(...args) {
            socket.end(...args);
        },

        write(...args) {
            socket.write(...args);
        },

        destroy(...args) {
            socket.destroy(...args);
        }
    });

    await new Promise(resolve => setTimeout(resolve, 2000));

    const session = http2.connect('https://nghttp2.org', {
        createConnection: () => socket
    });

    session.once('remoteSettings', () => {
        console.log('HTTP2 Received remote settings!');
    });

    session.once('connect', () => {
        console.log('HTTP2 Connected!');
    });

});

Additional information

master is not affected

@mmomtchev
Copy link
Contributor Author

switch (*p) {
doesn't seem to support %.* as a format specifier, yet there are a few examples in the code? @addaleax?
Currently the % eats the length specifier then the second argument triggers the CHECK_NOT_NULL

@mmomtchev mmomtchev changed the title Segfault with NODE_DEBUG_NATIVE=HTTP2SESSION %.* is not a supported debug specifier Oct 17, 2020
@addaleax
Copy link
Member

@mmomtchev Yeah, length specifiers make sense for a printf that is mainly intended to handle C strings, but it’s not important for a type-aware C++ variant like the one that we have. I think it would be just fine here to replace Debug(session, "Error '%.*s'", len, message); with Debug(session, "Error '%s'", std::string(message, len));.

@mmomtchev
Copy link
Contributor Author

Yes, in fact, there are 3 different sprintfs used through out the code: this one, from debug_utils-inl.h, the V8 one and the OS-provided one - and all the other examples are for the other two which support it

mmomtchev added a commit to mmomtchev/node that referenced this issue Oct 17, 2020
The debug sprintf doesn't support %.* specifiers

Fixes: nodejs#35688
@PoojaDurgad PoojaDurgad added http2 Issues or PRs related to the http2 subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Oct 19, 2020
targos pushed a commit that referenced this issue Nov 3, 2020
The debug sprintf doesn't support %.* specifiers

Fixes: #35688

PR-URL: #35694
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: David Carlier <[email protected]>
BethGriggs pushed a commit that referenced this issue Dec 8, 2020
The debug sprintf doesn't support %.* specifiers

Fixes: #35688

PR-URL: #35694
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: David Carlier <[email protected]>
BethGriggs pushed a commit that referenced this issue Dec 10, 2020
The debug sprintf doesn't support %.* specifiers

Fixes: #35688

PR-URL: #35694
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: David Carlier <[email protected]>
BethGriggs pushed a commit that referenced this issue Dec 15, 2020
The debug sprintf doesn't support %.* specifiers

Fixes: #35688

PR-URL: #35694
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ricky Zhou <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants