Skip to content

Fixed integer overflow issue with LMDB on 32bit systems.#4021

Closed
nickfraser wants to merge 4 commits intoBVLC:masterfrom
nickfraser:master
Closed

Fixed integer overflow issue with LMDB on 32bit systems.#4021
nickfraser wants to merge 4 commits intoBVLC:masterfrom
nickfraser:master

Conversation

@nickfraser
Copy link

I tried compiling Caffe on the Xilinx ZC706 evaluation board and noticed it wouldn't pass all of the tests. I then noticed that I have the same error when compiling Caffe on a 32bit Ubuntu 14.04 system (x86). It occurs with and without debugging enabled. It's likely this bug occurs in any system which uses an address size less than 41bits. I traced back the 1TB LMDB map size allocation in a few files. This 1TB allocation assumes that the underlying size_t parameter needed by LMDB is at least 41bits - this is not true on 32bit systems.

In order to address (if you'll excuse the pun) this, I did the following:

  1. I created a macro to saturate the DB allocation size to the largest value available on the current system, if the user specifies a larger value. (Perhaps this should also include a warning? Either way, this is more desirable than overflow in my opinion...)
  2. I added an extra variable in Makefile.config.example to allow the user to specify the desired allocation size. I added the definition of this symbol to the COMMON_FLAGS parameter in the Makefile.

The code should be self-explanatory, but let me know if you want any more information.

Also, if you have any suggestions on how I can improve my code - please let me know. Even if it's just a style preference - I'd like to hear about it.

Cheers,
Nick

@nickfraser nickfraser changed the title Fixed interger overflow issue with LMDB on 32bit systems. Fixed integer overflow issue with LMDB on 32bit systems. Apr 20, 2016
@seanbell
Copy link

seanbell commented Apr 20, 2016

Please see #3731, which is a more general solution to this problem.

@nickfraser
Copy link
Author

Apologies, I didn't see that pull request. I agree it's a better solution. However, the simplicity of this solution is less likely to create insidious bugs - if it were used temporarily until #3731 is merged.

@longjon
Copy link
Contributor

longjon commented May 4, 2016

Closing as #3731 has been merged... comment if there's an outstanding issue that I've missed.

@longjon longjon closed this May 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants