Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add ProcessInfo2 call #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonswine
Copy link

@simonswine simonswine commented Jan 17, 2024

This implements the ProcessInfo2 call, to figure out more information about the process and runtime

Copy link
Member

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

Looks good! There's one minor issue with the parser error check – please take a look

Comment on lines +103 to +127
buf := bytes.NewBuffer(nil)
if _, err := io.CopyN(buf, conn, int64(header.Size-headerSize)); err != nil {
return nil, err
}

if err := binary.Read(buf, binary.LittleEndian, &resp.ProcessID); err != nil {
return nil, fmt.Errorf("unable to read process ID: %w", err)
}

if err := binary.Read(buf, binary.LittleEndian, &resp.GUID); err != nil {
return nil, fmt.Errorf("unable to read process ID: %w", err)
}

// now parse the strings out
p := &nettrace.Parser{Buffer: buf}
p.UTF16NTS()
resp.CommandLine = p.UTF16NTS()
p.UTF16NTS()
resp.OS = p.UTF16NTS()
p.UTF16NTS()
resp.Arch = p.UTF16NTS()
p.UTF16NTS()
resp.AssemblyName = p.UTF16NTS()
p.UTF16NTS()
resp.RuntimeVersion = p.UTF16NTS()
Copy link
Member

Choose a reason for hiding this comment

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

We need to check the parser error with p.Err() in the end

nit:

It looks like the buffer size is known in advance, probably we should pre-alloc it. Also, for small payloads of known size io.ReadAtLeast might be a better alternative to io.Copy. Provided that we will be calling ProcessInfo2 relatively frequently, this micro-optimisation might make sense. Also, we could parse all fields with parser, like:

	md := Metadata{p: &Parser{Buffer: blob.Payload}}
	md.p.Read(&md.Header.MetaDataID)
	md.Header.ProviderName = md.p.UTF16NTS()
	md.p.Read(&md.Header.EventID)
	md.Header.EventName = md.p.UTF16NTS()
	md.p.Read(&md.Header.Keywords)
	md.p.Read(&md.Header.Version)
	md.p.Read(&md.Header.Level)

There's probably a bug in the UTF16NTS() function, as it should be called once per a string:

  • It looks like the fallback path (decodeUTF16NTS) only works as intended if the very first char is a non-ASCII one
  • the function is quite wasteful: we probably should reuse bytes buffer / use strings.Buidler

Comment on lines +99 to +101
if header.CommandSet != CommandSetServer || header.CommandID != 00 {
return nil, fmt.Errorf("unexpected response header: commandSet=%v (expected 0xff) commandID=%v (expected 0x00)", header.CommandSet, header.CommandID)
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could get the error code when CommandID is 0xFF

In readResponse we have quite weak check, and a TODO note – I'd fix this:

func verifyResponseHeader(h Header) error {
	if h.CommandSet != CommandSetServer {
		return  fmt.Errorf("%w: received response with unknown command set %#x", ErrDiagnosticServer, h.CommandSet)
	}
	switch h.CommandID {
	default:
		return fmt.Errorf("%w: received response with unknown command ID %#x", ErrDiagnosticServer, h.CommandID)
	case 0x00:
		return nil
	case 0xFF:
		var er ErrorResponse
		if err := binary.Read(r, binary.LittleEndian, &er); err != nil {
			return err
		}
		return fmt.Errorf("%w: error code %#x", ErrDiagnosticServer, er.Code)
	}
}

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants