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.

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

Undo commit bfccd62599ef80899929cbbdf7b8f0fc78f8f07b #677

Merged
merged 1 commit into from
Apr 14, 2018
Merged

Undo commit bfccd62599ef80899929cbbdf7b8f0fc78f8f07b #677

merged 1 commit into from
Apr 14, 2018

Conversation

Daniel-Abrecht
Copy link
Contributor

Reverting commit bfccd62 because it caused a crash in my fuse-nfs builds,
probably because the buffer size given to strncpy was wrong.
Why was this changed in the first place, the additional bound check
seams unnecessary, and it wasn't made sure the string would get null
terminated if it would ever somehow prevent an overflow. The only
way I see this could happen would be if the "opt" argument was changed
by a different thread in this tiny moment between strlen and strcpy,
if that's even possible, and even then, why would anyone ever do that,
an attacker would already have control over the program at that stage.

@Liryna
Copy link
Member

Liryna commented Apr 11, 2018

Hi @Daniel-Abrecht ,

Thank you for your contribution.

Using strcpy is generally not recommended (code analyzer tools). I understand that here it is safe to use it but since was not hurting to change it I did. I probably failed in the len calculus, this need to be reviewed. Do you think, you could look at which one has an issue ?

@Daniel-Abrecht
Copy link
Contributor Author

Daniel-Abrecht commented Apr 11, 2018

Both length calculations are wrong. You passed the length of the whole buffer to strncpy, but you passed the buffer with an offset. You would have to substract that offset from the length of the buffer to get the remaining length of the buffer starting from that offset. But I think it's easier and safer to just use the length of the new option/param and add the nullbyte manually.

Here is a commit for this:
Daniel-Abrecht@aafbf4a

I'm never quiet sure how I should add such changes, if I add it to this pull request it would leaf an unnecessary commit in the git history, but if I open another pull request the discussion get's split into two parts. Can github handle a force push to a commit used to start a pull request? But then the other commit would be lost...

@Rondom
Copy link
Contributor

Rondom commented Apr 14, 2018

GitHub handles force pushes to pull requests fine and will at least nowadays keep the history as well. So one can still see the old commit for reference.

@Daniel-Abrecht
Copy link
Contributor Author

Ok, I've now made a force push. The new patch just corrects the length calculation and adds the null byte to terminate the strings explicitly. The previous commit seams to have been lost due to that though, it seams github is still not able to retain them if it's the first commit of a pull request.

@Liryna Liryna merged commit c60725b into dokan-dev:master Apr 14, 2018
@Liryna
Copy link
Member

Liryna commented Apr 14, 2018

Hi @Daniel-Abrecht ,

Looks good to me 👍
Thank you for the report again and for providing a quick fix in the same time 🥇 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants