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.

Sign up for GitHub

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

Correct sign-compare warnings #446

Merged
merged 4 commits into from
Nov 18, 2017
+16 −16
Merged

Correct sign-compare warnings #446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5eb521
Correct sign-compare warnings
Nov 2, 2017
cd457f3
Merge branch 'master' into warn4
Nov 18, 2017
cf3de75
Dummy commit
Nov 18, 2017
411e6d9
to make the test run again
Nov 18, 2017
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
16 changes: 8 additions & 8 deletions encfs/BlockFileIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ inline Type min(Type A, Type B) {
return (B < A) ? B : A;
}

static void clearCache(IORequest &req, int blockSize) {
static void clearCache(IORequest &req, unsigned int blockSize) {
memset(req.data, 0, blockSize);
req.dataLen = 0;
}

BlockFileIO::BlockFileIO(int blockSize, const FSConfigPtr &cfg)
BlockFileIO::BlockFileIO(unsigned int blockSize, const FSConfigPtr &cfg)
: _blockSize(blockSize), _allowHoles(cfg->config->allowHoles) {
CHECK(_blockSize > 1);
_cache.data = new unsigned char[_blockSize];
Expand Down Expand Up @@ -89,7 +89,7 @@ ssize_t BlockFileIO::cacheReadOneBlock(const IORequest &req) const {
if (result > 0) {
_cache.offset = req.offset;
_cache.dataLen = result; // the amount we really have
if (result > req.dataLen) {
if ((size_t)result > req.dataLen) {
result = req.dataLen; // only as much as requested
}
memcpy(req.data, _cache.data, result);
Expand Down Expand Up @@ -146,7 +146,7 @@ ssize_t BlockFileIO::read(const IORequest &req) const {

// if we're reading a full block, then read directly into the
// result buffer instead of using a temporary
if (partialOffset == 0 && size >= (size_t)_blockSize) {
if (partialOffset == 0 && size >= _blockSize) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't _blockSize be unsigned from the start?

Copy link
Contributor Author

@benrubson benrubson Nov 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It is unsigned now, thanks to this PR.
Here I then remove a cast which is no more needed, as size_t is unsigned.

blockReq.data = out;
} else {
if (mb.data == nullptr) {
Expand All @@ -164,8 +164,8 @@ ssize_t BlockFileIO::read(const IORequest &req) const {
break; // didn't get enough bytes
}

size_t cpySize = min((size_t)(readSize - partialOffset), size);
CHECK(cpySize <= readSize);
size_t cpySize = min((size_t)readSize - (size_t)partialOffset, size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't partialOffset be unsigned?

Copy link
Contributor Author

@benrubson benrubson Nov 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Literally yes, but as req.offset is signed (from which it is calculated), I would say no.
In addition, AFAIR, min() expect 2 members of the same type, so cast would be required.

CHECK(cpySize <= (size_t)readSize);

// if we read to a temporary buffer, then move the data
if (blockReq.data != out) {
Expand Down Expand Up @@ -256,7 +256,7 @@ ssize_t BlockFileIO::write(const IORequest &req) {
// if writing an entire block, or writing a partial block that requires
// no merging with existing data..
if ((toCopy == _blockSize) ||
(partialOffset == 0 && blockReq.offset + toCopy >= fileSize)) {
(partialOffset == 0 && blockReq.offset + (off_t)toCopy >= fileSize)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

toCopy is not signed, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, so without the cast this leads to a comparison between size_t and off_t :
comparison of integers of different signs: 'unsigned long' and 'off_t' (aka 'long') [clang-diagnostic-sign-compare]

// write directly from buffer
blockReq.data = inPtr;
blockReq.dataLen = toCopy;
Expand Down Expand Up @@ -314,7 +314,7 @@ ssize_t BlockFileIO::write(const IORequest &req) {
return req.dataLen;
}

int BlockFileIO::blockSize() const { return _blockSize; }
unsigned int BlockFileIO::blockSize() const { return _blockSize; }

/**
* Returns 0 in case of success, or -errno in case of failure.
Expand Down
6 changes: 3 additions & 3 deletions encfs/BlockFileIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ namespace encfs {
*/
class BlockFileIO : public FileIO {
public:
BlockFileIO(int blockSize, const FSConfigPtr &cfg);
BlockFileIO(unsigned int blockSize, const FSConfigPtr &cfg);
virtual ~BlockFileIO();

// implemented in terms of blocks.
virtual ssize_t read(const IORequest &req) const;
virtual ssize_t write(const IORequest &req);

virtual int blockSize() const;
virtual unsigned int blockSize() const;

protected:
int truncateBase(off_t size, FileIO *base);
Expand All @@ -59,7 +59,7 @@ class BlockFileIO : public FileIO {
ssize_t cacheReadOneBlock(const IORequest &req) const;
ssize_t cacheWriteOneBlock(const IORequest &req);

int _blockSize;
unsigned int _blockSize;
bool _allowHoles;
bool _noCache;

Expand Down
6 changes: 3 additions & 3 deletions encfs/CipherFileIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ ssize_t CipherFileIO::writeOneBlock(const IORequest &req) {
return -EPERM;
}

int bs = blockSize();
unsigned int bs = blockSize();
off_t blockNum = req.offset / bs;

if (haveHeader && fileIV == 0) {
Expand Down Expand Up @@ -535,7 +535,7 @@ ssize_t CipherFileIO::read(const IORequest &origReq) const {
* to the data. */
if (req.offset < 0) {
headerBytes = -req.offset;
if (req.dataLen < headerBytes) {
if (req.dataLen < (size_t)headerBytes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't headerBytes be unsigned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, but HEADER_SIZE itself is a int...
Moving headerBytes to unsigned int would then lead to some limit considerations, for example line 545.
You point to #447 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an additional look, if we make HEADER_SIZE unsigned, other casts will be needed as it is compared to st_size which is an off_t...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. Maybe we should move everything to ssize_t or int64_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought about it, but I think dataLen should remain size_t, as in read(2) and write(2).

headerBytes = req.dataLen; // only up to the number of bytes requested
}
VLOG(1) << "Adding " << headerBytes << " header bytes";
Expand All @@ -546,7 +546,7 @@ ssize_t CipherFileIO::read(const IORequest &origReq) const {
memcpy(req.data, &headerBuf[headerOffset], headerBytes);

// the read does not want data beyond the header
if (headerBytes == req.dataLen) {
if ((size_t)headerBytes == req.dataLen) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok I looked through all of the casts again. Everything seems safe except this one. It looks like headerBytes may be negative at this point. Rather cast req.dataLen to int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmmh I'm not sure how headerBytes could be negative.
Which path would make it negative ?

I think casting size_t to int is rather dangerous.

Thx 👍

return headerBytes;
}

Expand Down
2 changes: 1 addition & 1 deletion encfs/FileIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ FileIO::FileIO() = default;

FileIO::~FileIO() = default;

int FileIO::blockSize() const { return 1; }
unsigned int FileIO::blockSize() const { return 1; }

bool FileIO::setIV(uint64_t iv) {
(void)iv;
Expand Down
2 changes: 1 addition & 1 deletion encfs/FileIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class FileIO {
virtual Interface interface() const = 0;

// default implementation returns 1, meaning this is not block oriented.
virtual int blockSize() const;
virtual unsigned int blockSize() const;

virtual void setFileName(const char *fileName) = 0;
virtual const char *getFileName() const = 0;
Expand Down