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

Check that dest is valid for decompression #3555

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contrib/linux-kernel/mem.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ typedef uint32_t U32;
typedef int32_t S32;
typedef uint64_t U64;
typedef int64_t S64;
typedef uintptr_t uptrval;

/*-**************************************************************
* Memory I/O API
Expand Down
2 changes: 2 additions & 0 deletions lib/common/mem.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ extern "C" {
typedef int32_t S32;
typedef uint64_t U64;
typedef int64_t S64;
typedef uintptr_t uptrval;
#else
# include <limits.h>
#if CHAR_BIT != 8
Expand All @@ -81,6 +82,7 @@ extern "C" {
* limits exist in C99, however, in such case, <stdint.h> is preferred */
typedef unsigned long long U64;
typedef signed long long S64;
typedef size_t uptrval; /* generally true, except OpenVMS-64 */
#endif


Expand Down
3 changes: 3 additions & 0 deletions lib/decompress/zstd_decompress_block.c
Original file line number Diff line number Diff line change
Expand Up @@ -2125,6 +2125,9 @@ ZSTD_decompressBlock_internal(ZSTD_DCtx* dctx,
srcSize -= seqHSize;

RETURN_ERROR_IF(dst == NULL && nbSeq > 0, dstSize_tooSmall, "NULL not handled");
RETURN_ERROR_IF(dstCapacity == 0 && nbSeq > 0, dstSize_tooSmall, "dstCapacity == 0, but nbSeq is non-zero");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can combine this check with the line 2127. dst may only be NULL if dstCapacity == 0.

RETURN_ERROR_IF(MEM_64bits() && (uptrval)dst + dstCapacity > (uptrval)-1 - (1 << 20), dstSize_tooSmall,
Copy link
Contributor

Choose a reason for hiding this comment

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

@Cyan4973 what do you think of requiring uintptr_t here? We already do the same in LZ4.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm generally not a fan of uintptr_t, because it's an optional type,
and we learned the hard way that this type breaks on some platforms.

What about the following alternative test ?

#define MAX_ADDRESS (const char*)(size_t)(-1)
#define SAFE_DISTANCE  (1 << 20)

if (MAX_ADDRESS - (const char*)dst < SAFE_DISTANCE) { ... } 

"invalid dst");

/* If we could potentially have long offsets, or we might want to use the prefetch decoder,
* compute information about the share of long offsets, and the maximum nbAdditionalBits.
Expand Down
6 changes: 5 additions & 1 deletion tests/fuzz/block_decompress.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* decompression function to ensure the decompressor never crashes.
*/

#include "fuzz_data_producer.h"
#define ZSTD_STATIC_LINKING_ONLY

#include <stddef.h>
Expand All @@ -28,11 +29,12 @@ static size_t bufSize = 0;
int LLVMFuzzerTestOneInput(const uint8_t *src, size_t size)
{
size_t const neededBufSize = ZSTD_BLOCKSIZE_MAX;
FUZZ_dataProducer_t *producer = FUZZ_dataProducer_create(src, size);

/* Allocate all buffers and contexts if not already allocated */
if (neededBufSize > bufSize) {
free(rBuf);
rBuf = FUZZ_malloc(neededBufSize);
rBuf = FUZZ_malloc_rand(neededBufSize, producer);
bufSize = neededBufSize;
}
if (!dctx) {
Expand All @@ -42,6 +44,8 @@ int LLVMFuzzerTestOneInput(const uint8_t *src, size_t size)
ZSTD_decompressBegin(dctx);
ZSTD_decompressBlock(dctx, rBuf, neededBufSize, src, size);

FUZZ_dataProducer_free(producer);

#ifndef STATEFUL_FUZZING
ZSTD_freeDCtx(dctx); dctx = NULL;
#endif
Expand Down
1 change: 1 addition & 0 deletions tests/fuzz/fuzz_data_producer.c
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* You may select, at your option, one of the above-listed licenses.
*/

#include "fuzz_helpers.h"
#include "fuzz_data_producer.h"

struct FUZZ_dataProducer_s{
Expand Down
1 change: 0 additions & 1 deletion tests/fuzz/fuzz_data_producer.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
#include <stdio.h>
#include <stdlib.h>

#include "fuzz_helpers.h"

/* Struct used for maintaining the state of the data */
typedef struct FUZZ_dataProducer_s FUZZ_dataProducer_t;
Expand Down
16 changes: 16 additions & 0 deletions tests/fuzz/fuzz_helpers.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,22 @@ void* FUZZ_malloc(size_t size)
return NULL;
}

void* FUZZ_malloc_rand(size_t size, FUZZ_dataProducer_t *producer)
{
if (size > 0) {
void* const mem = malloc(size);
FUZZ_ASSERT(mem);
return mem;
} else {
uintptr_t ptr = 0;
/* Add +- 1M 50% of the time */
if (FUZZ_dataProducer_uint32Range(producer, 0, 1))
FUZZ_dataProducer_int32Range(producer, -1000000, 1000000);
return (void*)ptr;
}

}

int FUZZ_memcmp(void const* lhs, void const* rhs, size_t size)
{
if (size == 0) {
Expand Down
7 changes: 7 additions & 0 deletions tests/fuzz/fuzz_helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "fuzz.h"
#include "xxhash.h"
#include "zstd.h"
#include "fuzz_data_producer.h"
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
Expand Down Expand Up @@ -62,6 +63,12 @@ extern "C" {
*/
void* FUZZ_malloc(size_t size);

/**
* malloc except returns random pointer for zero sized data and FUZZ_ASSERT
* that malloc doesn't fail.
*/
void* FUZZ_malloc_rand(size_t size, FUZZ_dataProducer_t *producer);

/**
* memcmp but accepts NULL.
*/
Expand Down