Clean up some warnings by ethomson · Pull Request #4950 · libgit2/libgit2

ethomson · 2019-01-20T20:42:57Z

This cleans up some warnings encountered building on Win32 with MSVC. This PR attacks all the x86 warnings. (Some remain on amd64.)

src/blob.c

pks-t · 2019-01-23T10:59:13Z

src/diff_generate.c

 	git_index_entry entry;

+	if (!git__is_sizet(size)) {
+		giterr_set(GITERR_NOMEMORY, "file size overflow (for 32-bits) on '%s'", path);


Normally I'd say that this error message sucks. But you're just going with what we had previously, so I'm okay to stick with it

src/notes.c

tests/core/zstream.c

src/buffer.c

pks-t · 2019-01-23T11:32:48Z

src/transports/git.c


-	error = git_stream_write(s->io, request.ptr, request.size, 0);
+	write_size = min(request.size, INT_MAX);
+	error = (int)git_stream_write(s->io, request.ptr, write_size, 0);


Same here, right? We silently drop everything that's after the first INT_MAX bytes.

src/delta.c

pks-t · 2019-01-23T11:44:42Z

src/describe.c

-	int abbreviated_size)
+	unsigned int abbreviated_size)
 {
 	size_t size = abbreviated_size;


One might wonder whether this is the right thing to do. Previously, passing -1 was underflowing to a huge size, essentially saying "Do not abbreviate but print all digits". But the only callsite show_suffix gets called with git_describe_format_options.abbreviated_size, which is an unsigned int itself.

src/diff_generate.c

src/filter.c

Quiet down a warning from MSVC about how we're potentially losing data.

Quiet down a warning from MSVC about how we're potentially losing data. This cast is safe since we've explicitly tested that `strip_len` <= `last_len`.

Quiet down a warning from MSVC about how we're potentially losing data. This is safe since we've explicitly tested that it's within the range of 0-100.

Quiet down a warning from MSVC about how we're potentially losing data. This is safe since we've explicitly tested that it's positive and less than SIZE_MAX.

Quiet down a warning from MSVC about how we're potentially losing data. Ensure that we're within a uint16_t before we do.

Our blob size is a `git_off_t`, which is a signed 64 bit int. This may be erroneously negative or larger than `SIZE_MAX`. Ensure that the blob size fits into a `size_t` before casting.

The filesystem iterator takes `stat` data from disk and puts them into index entries, which use 32 bit ints for time (the seconds portion) and filesize. However, on most systems these are not 32 bit, thus will typically invoke a warning. Most users ignore these fields entirely. Diff and checkout code do use the values, however only for the cache to determine if they should check file modification. Thus, this is not a critical error (and will cause a hash recomputation at worst).

Cast actual filesystem data to the int32_t that index entries store.

A number of source files have their implementation #ifdef'd out (because they target another platform). MSVC warns on empty compilation units (with warning LNK4221). Ignore warning 4221 when creating the object library.

Quiet down a warning from MSVC about how we're potentially losing data. This is safe since we've explicitly tested it.

Our streams implementation takes a `size_t` that indicates the length of the data buffer to be written, and returns an `ssize_t` that indicates the length that _was_ written. Clearly no such implementation can write more than `SSIZE_MAX` bytes. Ensure that each TLS stream implementation does not try to write more than `SSIZE_MAX` bytes (or smaller; if the given implementation takes a smaller size).

Windows doesn't include ssize_t or its _MAX value by default. We are already declaring ssize_t as SSIZE_T, which is __int64_t on Win64 and long otherwise. Include its _MAX value as a correspondence to its type.

The transport code returns an `int` with the number of bytes written; thus only attempt to write at most `INT_MAX`.

Quiet down a warning from MSVC about how we're potentially losing data. Validate that our data will fit into the type provided then cast.

The git_describe_format_options.abbreviated_size type is an unsigned int. There's no need for it to be anything else; keep it what it is.

Index entries are 32 bit unsigned ints, not `size_t`s.

Validate that the return value of the read is not less than INT_MAX, then cast.

ethomson · 2019-01-25T23:24:10Z

OK, fixed up but I'll wait for #4953 to land.

ethomson · 2019-01-26T19:43:30Z

On second thought, it's only the SSIZE_MAX, so that's a pretty trivial conflict. I want to look at the amd64 warnings, so 🚢

ethomson force-pushed the ethomson/warnings branch from 5ef24f1 to def4fd0 Compare January 21, 2019 11:49

pks-t reviewed Jan 23, 2019

View reviewed changes

pks-t mentioned this pull request Jan 23, 2019

stream: fix potentially truncated writes #4953

Closed

ethomson added 20 commits January 25, 2019 22:31

diff: explicitly cast in flush_hunk

c3866fa

Quiet down a warning from MSVC about how we're potentially losing data.

index: explicitly cast down to a size_t

494448a

Quiet down a warning from MSVC about how we're potentially losing data. This cast is safe since we've explicitly tested that `strip_len` <= `last_len`.

patch: explicitly cast down in parse_header_percent

80c3867

Quiet down a warning from MSVC about how we're potentially losing data. This is safe since we've explicitly tested that it's within the range of 0-100.

odb_loose: explicitly cast to size_t

759502e

Quiet down a warning from MSVC about how we're potentially losing data. This is safe since we've explicitly tested that it's positive and less than SIZE_MAX.

tree: cast filename length in git_tree__parse_raw

3aa6d96

Quiet down a warning from MSVC about how we're potentially losing data. Ensure that we're within a uint16_t before we do.

blob: validate that blob sizes fit in a size_t

c6cac73

Our blob size is a `git_off_t`, which is a signed 64 bit int. This may be erroneously negative or larger than `SIZE_MAX`. Ensure that the blob size fits into a `size_t` before casting.

index test: cast times explicitly

f25bb50

Cast actual filesystem data to the int32_t that index entries store.

test: cast to a char the zstream test

3fba589

MSVC: ignore empty compilation units (warning LNK4221)

7ed2baf

A number of source files have their implementation #ifdef'd out (because they target another platform). MSVC warns on empty compilation units (with warning LNK4221). Ignore warning 4221 when creating the object library.

apply: make update_hunk accept a size_t

ae681d3

blame: make hunk_cmp handle unsigned differences

f4ebb2d

buffer: explicitly cast

e5e2fac

Quiet down a warning from MSVC about how we're potentially losing data. This is safe since we've explicitly tested it.

windows: add SSIZE_MAX

a861839

Windows doesn't include ssize_t or its _MAX value by default. We are already declaring ssize_t as SSIZE_T, which is __int64_t on Win64 and long otherwise. Include its _MAX value as a correspondence to its type.

git transport: only write INT_MAX bytes

4947216

The transport code returns an `int` with the number of bytes written; thus only attempt to write at most `INT_MAX`.

delta: validate sizes and cast safely

751eb46

Quiet down a warning from MSVC about how we're potentially losing data. Validate that our data will fit into the type provided then cast.

describe: don't mix and match abbreviated size types

fd9d4e2

The git_describe_format_options.abbreviated_size type is an unsigned int. There's no need for it to be anything else; keep it what it is.

diff_generate: validate oid file size

89bd4dd

Index entries are 32 bit unsigned ints, not `size_t`s.

filter: return an int

fac0883

Validate that the return value of the read is not less than INT_MAX, then cast.

ethomson force-pushed the ethomson/warnings branch from def4fd0 to fac0883 Compare January 25, 2019 22:48

ethomson changed the title ~~WIP: clean up some warnings~~ Clean up some warnings Jan 25, 2019

ethomson added the next release label Jan 25, 2019

ethomson merged commit b1e2862 into master Jan 26, 2019

ethomson deleted the ethomson/warnings branch January 27, 2019 22:47

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up some warnings#4950

Clean up some warnings#4950
ethomson merged 20 commits intomasterfrom
ethomson/warnings

ethomson commented Jan 20, 2019 •

edited

Loading

Uh oh!

Uh oh!

pks-t Jan 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t Jan 23, 2019

Uh oh!

ethomson Jan 23, 2019

Uh oh!

Uh oh!

pks-t Jan 23, 2019

Uh oh!

Uh oh!

Uh oh!

ethomson commented Jan 25, 2019

Uh oh!

ethomson commented Jan 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ethomson commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pks-t Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

ethomson Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pks-t Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ethomson commented Jan 25, 2019

Uh oh!

ethomson commented Jan 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ethomson commented Jan 20, 2019 •

edited

Loading