Azure / azure-sdk-for-go

This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at:
https://docs.microsoft.com/azure/developer/go/
MIT License
1.58k stars 816 forks source link

Blob client `SetTags` panics if given an empty `tags map[string]string` #22171

Closed LasseHels closed 8 months ago

LasseHels commented 8 months ago

Bug Report

Import path: github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob.

SDK version: v1.1.0.

Go version: go version go1.21.0 darwin/arm64.


When calling the func (b *Client) SetTags(ctx context.Context, tags map[string]string, options *SetTagsOptions) (SetTagsResponse, error) method with an empty tags map, the method panics:

=== RUN   TestStorageAccount/TestStorageAccount_SetTagsAcceptsEmptyInput
    suite.go:87: test panicked: runtime error: invalid memory address or nil pointer dereference
        goroutine 248 [running]:
        runtime/debug.Stack()
            /opt/homebrew/Cellar/go/1.21.0/libexec/src/runtime/debug/stack.go:24 +0x64
        github.com/stretchr/testify/suite.failOnPanic(0x14000682680, {0x101308dc0, 0x101854de0})
            /Users/lasse.hels/go/pkg/mod/github.com/stretchr/testify@v1.8.4/suite/suite.go:87 +0x34
        github.com/stretchr/testify/suite.Run.func1.1()
            /Users/lasse.hels/go/pkg/mod/github.com/stretchr/testify@v1.8.4/suite/suite.go:183 +0x1f8
        panic({0x101308dc0?, 0x101854de0?})
            /opt/homebrew/Cellar/go/1.21.0/libexec/src/runtime/panic.go:914 +0x218
        github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob.(*Client).SetTags(0x1400050cee8?, {0x10140b730?, 0x1018e26c0?}, 0x140004a7d60?, 0x140004a7d70?)
            /Users/lasse.hels/go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/storage/azblob@v1.1.0/blob/client.go:223 +0x110
        github.com/Maersk-Global/observability-the-pensieve-snitch/pkg/azure.(*StorageAccount).SetTags(0x14000371c00?, {0x10140b730, 0x1018e26c0}, {0x10114f0e2?, 0xd?}, {0x10114c05b, 0x8}, 0x0?)
            /Users/lasse.hels/Projects/observability-the-pensieve-snitch/pkg/azure/storageaccount.go:144 +0x7c
        github.com/Maersk-Global/observability-the-pensieve-snitch/pkg/azure_test.(*IntegrationTestSuite).TestStorageAccount_SetTagsAcceptsEmptyInput(0x14000371c00)
            /Users/lasse.hels/Projects/observability-the-pensieve-snitch/pkg/azure/storageaccount_test.go:254 +0x9c
        reflect.Value.call({0x140003c6340?, 0x1400006c620?, 0x140005f5548?}, {0x10114a523, 0x4}, {0x1400007de68, 0x1, 0x148ad0f38?})
            /opt/homebrew/Cellar/go/1.21.0/libexec/src/reflect/value.go:596 +0x994
        reflect.Value.Call({0x140003c6340?, 0x1400006c620?, 0x14000371c00?}, {0x1400007de68?, 0x10143f5bf?, 0xf?})
            /opt/homebrew/Cellar/go/1.21.0/libexec/src/reflect/value.go:380 +0x94
        github.com/stretchr/testify/suite.Run.func1(0x14000682680)
            /Users/lasse.hels/go/pkg/mod/github.com/stretchr/testify@v1.8.4/suite/suite.go:197 +0x378
        testing.tRunner(0x14000682680, 0x140003fbef0)
            /opt/homebrew/Cellar/go/1.21.0/libexec/src/testing/testing.go:1595 +0xe8
        created by testing.(*T).Run in goroutine 24
            /opt/homebrew/Cellar/go/1.21.0/libexec/src/testing/testing.go:1648 +0x33c

The expected behaviour is that the method can be called with an empty tags map to remove all tags from the given blob. The method signature itself explicitly states this:

// SetTags operation enables users to set tags on a blob or specific blob version, but not snapshot. // Each call to this operation replaces all existing tags attached to the blob. // To remove all tags from the blob, call this operation with no tags set. // https://docs.microsoft.com/en-us/rest/api/storageservices/set-blob-tags

As does the official API documentation:

The Set Blob Tags operation overwrites all existing tags on the blob. To remove all tags from a blob, send a Set Blob Tags request with an empty <TagSet>.

For reference, here is the code that I am using to trigger the panic:

response, err := client.SetTags(suite.ctx, map[string]string{}, &blob.SetTagsOptions{})

I believe the issue is that shared.SerializeBlobTags() returns nil if the provided map is empty:

    if len(tagsMap) == 0 {
        return nil
    }

This causes a nil pointer dereference later on:

resp, err := b.generated().SetTags(ctx, *serializedTags, blobSetTagsOptions, modifiedAccessConditions, leaseAccessConditions)

Thanks.

souravgupta-msft commented 8 months ago

@LasseHels, thanks for reaching out. This issue has been fixed in the latest release v1.2.1. Please update to the latest version.