Fix counter file race-condition vulnerability

[Brikaa]

Fixes #66

[antonkomarev]

@Brikaa you for the fix. I will try to look at it this weekend.

[antonkomarev]

Looks good, but I'm not sure we need to call ftruncate($counterFile, 0); because fwrite will overwrite first line anyway, and we don't have any content there.

[antonkomarev]

@Brikaa I've updated your solution:

• 68

[Brikaa]

@antonkomarev brilliant updates, thanks for pinging and explaining your code in the PR. Good call on avoiding errors from getViewsCountByUsername. This is actually where I suspected the problem was from in the first place. And good call on releasing the lock in the finally block.

[Brikaa]

I was thinking if we need a lock for appending content to the views files. But I found this in PHP's fwrite documentation:

Note:

If stream was fopen()ed in append mode, fwrite()s are atomic ?(unless the size of data exceeds the filesystem's block size, on some platforms, and as long as the file is on a local filesystem). That is, there is no need to flock() a resource before calling fwrite(); all of the data will be written without interruption.

[antonkomarev]

@Brikaa we need a lock to avoid the second request to receive counter value before the first request is not finished.

[Brikaa]

@antonkomarev yeah, I was just wondering if we needed another lock on the views file.

The current implementation with one lock works because if the second request is another one trying to increment the counter, it will have to wait anyway since we read the counter after acquiring the lock.

I believe a problem that might happen is a deadlock if the system uses mandatory locking instead of advisory locking because we use file_get_contents after acquiring the lock and before releasing it. file_get_contents will be blocked.

[antonkomarev]

File is not locked for the file_get_contents.

[Brikaa]

If you mean the file_get_contents in the incrementViewsCount function, yes it does not respect the advisory lock. But it doesn't really matter since file_get_contents happens inside the "critical section". You can't reach this section unless you acquire a lock and you release the lock after this section.

Without mandatory locking instead of advisory locking I think that would cause a deadlock.

[antonkomarev]

Yes, we could release the lock earlier, but it looks like microseconds optimization.

[Brikaa]

@antonkomarev to understand what you mean, when do you think we should release the lock? In the incrementViewsCount function, we are doing:

• Acquire lock
• Read counter (file_get_contents)
• Increment counter
• Release lock

Both "Read counter" and "Increment counter" happen while the lock is acquired so there can't be race conditions despite the "Read counter" step using file_get_contents which doesn't respect the advisory lock.

[antonkomarev]

@Brikaa I'll try to draw a sequence diagram to visualize how 2 requests should work later this week. Do not have much free time now.

[antonkomarev]

@Brikaa i drawed pretty raw diagram just to visualize the lock

[Brikaa]

Yes, that's how they currently work. Were you saying that we needed a further improvement in the code?

@Brikaa we need a lock to avoid the second request to receive counter value before the first request is not finished.

[Brikaa]

Or were you just explaining how the current locking works in that quoted comment?

[antonkomarev]

@Brikaa I described why we need a lock. Right now it should work as I showed on the diagram. I don't see cases with a deadlock because of multiple simultaneous requests right now.

[Brikaa]

@antonkomarev ah yes, thanks. We don't disagree. I was not saying we do not need a lock for incrementing the counter, I was just saying we do not need one (and indeed we don't have one) for appending to the other views file because appending is atomic:

I was thinking if we need a lock for appending content to the views files. But I found this in PHP's fwrite documentation:

Note: If stream was fopen()ed in append mode, fwrite()s are atomic ?(unless the size of data exceeds the filesystem's block size, on some platforms, and as long as the file is on a local filesystem). That is, there is no need to flock() a resource before calling fwrite(); all of the data will be written without interruption.

About the deadlocks, I was just talking about a special case that might happen if we use a system in which flock does "mandatory locking" since that would cause file_get_contents to hang waiting for the lock to be released.