Skip to content

fix: do not dereference nil attributes in the example Filecmd - #662

Open
labeedahmad-debug wants to merge 1 commit into
pkg:masterfrom
labeedahmad-debug:fix/example-setstat-nil-attributes
Open

labeedahmad-debug wants to merge 1 commit into
pkg:masterfrom
labeedahmad-debug:fix/example-setstat-nil-attributes

Conversation

@labeedahmad-debug

Copy link
Copy Markdown

Fixes #661.

Request.Attributes() returns nil when the attribute blob does not hold every field its flags promise. That is deliberate — #325 / #328 replaced a panic inside the parser with a nil return — but request-example.go dereferences it:

if r.AttrFlags().Size {
	return file.Truncate(int64(r.Attributes().Size))
}

A SSH_FXP_SETSTAT that sets SSH_FILEXFER_ATTR_SIZE and carries fewer than eight bytes of attributes reaches that line with nil. The panic happens in a packetWorker goroutine, so the caller cannot recover it and the process ends. I found it by fuzzing a server whose Filecmd started from this example.

What this changes

  • request-example.go: answer ErrSSHFxBadMessage when Attributes() is nil, instead of dereferencing it.
  • request-attrs.go: say in the doc comment that Attributes() returns nil for malformed attributes, so callers know they have to check. The comment gave no hint, and the example taught the unchecked pattern.
  • request-example_test.go (new): a regression test that drives the example handler with a short attribute blob. It panics without the fix and passes with it. It calls the handler directly rather than through clientRequestServerPair, so it also runs on Windows and Plan 9.

The library's own parsing is unchanged; only the example and a doc comment.

Verification

go test -run TestInMemHandlerSetstatShortAttributes -v .
=== RUN   TestInMemHandlerSetstatShortAttributes
--- PASS: TestInMemHandlerSetstatShortAttributes (0.00s)

Reverting only the request-example.go hunk makes that test panic at request-example.go:160, which is the reported crash.

go test ./... is otherwise unchanged by this PR. Note that TestCleanPath already fails on master on windows/amd64, before and after this change, so it looks unrelated and platform-specific.

🤖 Generated with Claude Code

Request.Attributes returns nil when the attribute blob does not hold every field its flags
promise; that behaviour is deliberate (pkg#325, pkg#328). The example server dereferenced it anyway,
so a SETSTAT that sets SSH_FILEXFER_ATTR_SIZE with fewer than eight bytes of attributes panicked
the packet worker and ended the process. Servers written from the example inherit that.

The example now answers SSH_FX_BAD_MESSAGE instead, and the doc comment on Attributes says a nil
result is possible, so callers know to check it.

Fixes pkg#661

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@puellanivis puellanivis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please do not use Co-Authored-By for an LLM. Legally, an LLM cannot be an author in any copyright jurisdiction that I am aware of. Instead, it is suggested to use Assisted-by:: https://docs.kernel.org/process/coding-assistants.html#attribution

Comment thread request-example_test.go
Comment on lines +12 to +17
fs := InMemHandler().FileCmd.(*root)

_, err := fs.Filewrite(&Request{Method: "Put", Filepath: "/foo", Flags: sshFxfWrite | sshFxfCreat})
require.NoError(t, err)

err = fs.Filecmd(&Request{

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is no reason to type assert out to the concrete *root type here. We can just use the FilePut and ̀ FileCmd` fields as they’re designed to be used:

Suggested change
fs := InMemHandler().FileCmd.(*root)
_, err := fs.Filewrite(&Request{Method: "Put", Filepath: "/foo", Flags: sshFxfWrite | sshFxfCreat})
require.NoError(t, err)
err = fs.Filecmd(&Request{
fs := InMemHandler()
_, err := fs.FilePut.Filewrite(&Request{Method: "Put", Filepath: "/foo", Flags: sshFxfWrite | sshFxfCreat})
require.NoError(t, err)
err = fs.FileCmd.Filecmd(&Request{

Comment thread request-example.go
return ErrSSHFxBadMessage
}

return file.Truncate(int64(attrs.Size))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I’m going to recommend we provide a better example here, because if one were supporting more than one setstat operation, it would need to be repeated. Instead, let’s test the attributes up front:

		attrs := r.Attributes()
		if attrs == nil {
			// Something went wrong parsing attributes
			return ErrSSHFxBadMessage
		}

		if r.AttrFlags().Size {
			return file.Truncate(int64(attrs.Size))
		}

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.

Example server panics on a SETSTAT whose attributes are shorter than its flags promise (request-example.go:160)

2 participants