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

BrotliEncoderSetCustomDictionary() causes "false" OOM #500

Closed
ralfjunker opened this Issue Jan 31, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@ralfjunker

ralfjunker commented Jan 31, 2017

In the code below, BrotliEncoderSetCustomDictionary() causes a "false" OOM on compression, which causes the application to halt. Curiously, the OOM is not a real OOM. It only appears as such as a result of malloc(0) which returns NULL.

Without the BrotliEncoderSetCustomDictionary() call all works fine.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "brotli/encode.h"

int main()
{
  unsigned int i;
  uint8_t dictionary [0x1000];
  uint8_t originalBuffer [0x2000];
  uint8_t compressedBuffer [0x2000];
  size_t availIn, availOut;
  const uint8_t * nextIn;
  uint8_t *nextOut;
  BrotliEncoderState *s;
  BROTLI_BOOL r;

  /* Create random dictionary. */
  for (i = 0; i < sizeof(dictionary); i++) {
    dictionary[i] = rand() % 256;
  }

  /* Create an input buffer exactly two times the dictionary. */
  memmove(originalBuffer, dictionary, sizeof(dictionary));
  memmove(&originalBuffer[0x1000], dictionary, sizeof(dictionary));

  s = BrotliEncoderCreateInstance(NULL, NULL, NULL);
  if (!s)
    printf("Error creating Brotli encoder\n");

  /* Set the  custom dictionary. This causes the OOM during compression. */
  BrotliEncoderSetCustomDictionary(s, sizeof(dictionary), &dictionary);

  availIn = sizeof(originalBuffer);
  nextIn = originalBuffer;

  availOut = sizeof(compressedBuffer);
  nextOut = compressedBuffer;

  /* Because the OOM exits the application, this call does not return. */
  r = BrotliEncoderCompressStream(s,
    BROTLI_OPERATION_FINISH, &availIn, &nextIn, &availOut, &nextOut, NULL);
  if (r != BROTLI_TRUE)
    printf("Error compressing data\n");

  BrotliEncoderDestroyInstance(s);

  return 0;
}

The OOM happens in BrotliSplitBlock(), file block_splitter.c on line 137:

https://github.com/google/brotli/blob/master/enc/block_splitter.c#L137

I hope you can reproduce the issue. If you have questions, please let me know.

@eustas

This comment has been minimized.

Contributor

eustas commented Feb 1, 2017

Hello.

Thanks for the report, going to fix it ASAP.

NB: exit on OOM is considered normal for code in production; use -DBROTLI_ENCODER_CLEANUP_ON_OOM compiler option to allow encoder finish its work and cleanup after itself.

@ralfjunker

This comment has been minimized.

ralfjunker commented Feb 1, 2017

You are correct in saying that -DBROTLI_ENCODER_CLEANUP_ON_OOM works around the exit. However, compression still fails and BrotliEncoderCompressStream() returns BROTLI_FALSE.

Here is a solution I found to pass my tests: Don't call BROTLI_ALLOC if literals_count is 0. It's a small change to https://github.com/google/brotli/blob/master/enc/block_splitter.c#L137, just replace

uint8_t* literals = BROTLI_ALLOC(m, uint8_t, literals_count);

with

uint8_t* literals = literals_count ? BROTLI_ALLOC(m, uint8_t, literals_count) : NULL;

eustas added a commit that referenced this issue Feb 6, 2017

Update encoder
 * pull `BROTLI_MAX_BACKWARD_LIMIT` to constants
 * split generic and Zopfli backward references code
 * pull hashers init and stitch invocation to encoder
 * make `dictionary_hash` a compilation unit
 * add `size hint` parameter
 * add new hasher
 * use `size hint` to pick new hasher for q4
 * modernize clz guard (fix #495)
 * move `hash to binary tree` to separate file
 * add `Initialize` and `Cleanup` to all hashers
 * do not raise OOM if malloc(0) == NULL (fix #500)

@eustas eustas referenced this issue Feb 6, 2017

Closed

Update encoder #503

eustas added a commit that referenced this issue Feb 6, 2017

Update encoder
 * pull `BROTLI_MAX_BACKWARD_LIMIT` to constants
 * split generic and Zopfli backward references code
 * pull hashers init and stitch invocation to encoder
 * make `dictionary_hash` a compilation unit
 * add `size hint` parameter
 * add new hasher
 * use `size hint` to pick new hasher for q4
 * modernize clz guard (fix #495)
 * move `hash to binary tree` to separate file
 * add `Initialize` and `Cleanup` to all hashers
 * do not raise OOM if malloc(0) == NULL (fix #500)

@eustas eustas referenced this issue Feb 6, 2017

Merged

Update encoder #504

@eustas eustas closed this in #504 Feb 6, 2017

eustas added a commit that referenced this issue Feb 6, 2017

Update encoder (#504)
* pull `BROTLI_MAX_BACKWARD_LIMIT` to constants
 * split generic and Zopfli backward references code
 * pull hashers init and stitch invocation to encoder
 * make `dictionary_hash` a compilation unit
 * add `size hint` parameter
 * add new hasher
 * use `size hint` to pick new hasher for q4
 * modernize clz guard (fix #495)
 * move `hash to binary tree` to separate file
 * add `Initialize` and `Cleanup` to all hashers
 * do not raise OOM if malloc(0) == NULL (fix #500)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment