paleolimbot commented on code in PR #983:
URL: https://github.com/apache/arrow-nanoarrow/pull/983#discussion_r4349403663
##########
src/nanoarrow/common/utils.c:
##########
@@ -237,9 +237,9 @@ void ArrowLayoutInit(struct ArrowLayout* layout, enum
ArrowType storage_type) {
}
}
-void* ArrowMalloc(int64_t size) { return malloc(size); }
+void* ArrowMalloc(int64_t size) { return malloc((size_t)size); }
-void* ArrowRealloc(void* ptr, int64_t size) { return realloc(ptr, size); }
+void* ArrowRealloc(void* ptr, int64_t size) { return realloc(ptr,
(size_t)size); }
Review Comment:
I think these conversions should be checked (and return null on failure),
cleaning up the pointer in the realloc case
##########
src/nanoarrow/ipc/reader.c:
##########
@@ -66,7 +66,8 @@ static ArrowErrorCode ArrowIpcInputStreamBufferRead(struct
ArrowIpcInputStream*
}
if (bytes_to_read > 0) {
- memcpy(buf, private_data->input.data + private_data->cursor_bytes,
bytes_to_read);
+ memcpy(buf, private_data->input.data + private_data->cursor_bytes,
+ (size_t)bytes_to_read);
Review Comment:
I think this can be a NANOARROW_DCHECK that bytes_to_read is small enough
(there shouldn't have been a single in memory buffer bigger than 4 GB in the
process anyway)
##########
src/nanoarrow/ipc/reader.c:
##########
@@ -293,7 +295,7 @@ static int ArrowIpcArrayStreamReaderNextHeader(
NANOARROW_RETURN_NOT_OK_WITH_ERROR(ArrowBufferReserve(&private_data->header, 4),
&private_data->error);
memmove(private_data->header.data + 4, private_data->header.data,
- private_data->header.size_bytes);
+ (size_t)private_data->header.size_bytes);
Review Comment:
Let's check this and return an error if outside the bounds
##########
src/nanoarrow/ipc/encoder.c:
##########
@@ -250,7 +250,8 @@ ArrowErrorCode ArrowIpcEncoderFinalizeBuffer(struct
ArrowIpcEncoder* encoder,
int64_t padded_size = _ArrowRoundUpToMultipleOf8(size);
NANOARROW_RETURN_NOT_OK(
ArrowBufferReserve(out, sizeof(int32_t) + sizeof(int32_t) +
padded_size));
- NANOARROW_ASSERT_OK(ArrowIpcEncoderWriteContinuationAndSize(out,
padded_size));
+ NANOARROW_ASSERT_OK(
+ ArrowIpcEncoderWriteContinuationAndSize(out, (size_t)padded_size));
Review Comment:
We should check the padded size here (or update
ArrowIpcEncoderWriteContinuationAndSize() to accept an int64 and check the
failure code)
##########
src/nanoarrow/ipc/reader.c:
##########
@@ -133,7 +134,8 @@ static ArrowErrorCode ArrowIpcInputStreamFileRead(struct
ArrowIpcInputStream* st
}
// Do the read
- int64_t bytes_read = (int64_t)fread(buf, 1, buf_size_bytes,
private_data->file_ptr);
+ int64_t bytes_read =
+ (int64_t)fread(buf, 1, (size_t)buf_size_bytes, private_data->file_ptr);
Review Comment:
I think here we can also NANOARROW_DCHECK (because the output can't have
been allocated to a size bigger than 4 GB in a 32 bit process)
##########
src/nanoarrow/ipc/writer.c:
##########
@@ -123,7 +123,8 @@ static ArrowErrorCode ArrowIpcOutputStreamFileWrite(struct
ArrowIpcOutputStream*
}
// Do the write
- int64_t bytes_written = (int64_t)fwrite(buf, 1, buf_size_bytes,
private_data->file_ptr);
+ int64_t bytes_written =
+ (int64_t)fwrite(buf, 1, (size_t)buf_size_bytes, private_data->file_ptr);
Review Comment:
I think a DCHECK would be good here too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
