Skip to content

path.d: add overflow checks #4713

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

Merged
dlang-bot merged 1 commit into from
May 20, 2017
Merged

path.d: add overflow checks #4713

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 11 additions & 3 deletions std/path.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3913,7 +3913,10 @@ string expandTilde(string inputPath) nothrow
assert(last_char > 1);

// Reserve C memory for the getpwnam_r() function.
int extra_memory_size = 5 * 1024;
version (unittest)
uint extra_memory_size = 2;
else
uint extra_memory_size = 5 * 1024;
char* extra_memory;
scope(exit) free(extra_memory);

Expand All @@ -3937,11 +3940,16 @@ string expandTilde(string inputPath) nothrow
break;
}

if (errno != ERANGE)
if (errno != ERANGE &&
// On FreeBSD and OSX, errno can be left at 0 instead of set to ERANGE
errno != 0)
Copy link
Member

@schveiguy schveiguy Aug 5, 2016

Choose a reason for hiding this comment

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

Shouldn't this check come inside the if clause above? I mean, I didn't think errno had any specified meaning if your return from the syscall was 0. It's always "if this returns nonzero, check errno".

Copy link
Member Author

@WalterBright WalterBright May 20, 2017

Choose a reason for hiding this comment

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

The idea is to have a "not enough memory" error result in increasing the memory size, and that is different from some other error.

onOutOfMemoryError();

// extra_memory isn't large enough
extra_memory_size *= 2;
import core.checkedint : mulu;
bool overflow;
extra_memory_size = mulu(extra_memory_size, 2, overflow);
if (overflow) assert(0);
Copy link
Member

@MetaLang MetaLang Mar 28, 2017

Choose a reason for hiding this comment

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

I was going to leave a comment about how there should be an assert message, but is the message even printed when using assert(0)?

Copy link
Contributor

@JackStouffer JackStouffer Mar 28, 2017

Choose a reason for hiding this comment

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

yes

Copy link
Member

@jmdavis jmdavis Mar 28, 2017

Choose a reason for hiding this comment

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

The message is printed if not built with -release, because in that case, assert(0) is compiled in like any assertion would be and throws an AssertError. However, with -release, when assertions are compiled out, it becomes a HLT instruction, and there is no message.

Copy link
Member

@MetaLang MetaLang Mar 28, 2017

Choose a reason for hiding this comment

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

Is there a better solution here than just halting the program when there's an overflow? We're talking about people's programs potentially crashing and there being no indication as to what happened.

Copy link
Contributor

@JackStouffer JackStouffer Mar 28, 2017

Choose a reason for hiding this comment

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

Well, you could throw an exception, or you could throw a static error singleton as Andrei has proposed before. Both come with their own problems.

Copy link
Contributor

@JackStouffer JackStouffer Mar 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Andrei kind of halted Walter's addition of overflow checks with his checked int module because he believed that was the right way to do things. I don't really see why these need to be stopped in the mean time though

}
return path;
}
Expand Down