Problem/Motivation

core/modules/file/src/Plugin/rest/resource/FileUploadResource.php will call fclose(FALSE); if it fails to open a temp file for storing the in-progress upload:

    $temp_file_path = $this->fileSystem->tempnam('temporary://', 'file');
    $temp_file = fopen($temp_file_path, 'wb');

    if ($temp_file) {
      ...
    }
    else {
      // Close the file streams.
      fclose($temp_file);  // Not a good idea.
      ...
    }

I first noticed this at #2940383-59: Unify file upload logic of REST and JSON:API.6. Then that issue got postponed, and JSON:API is moving forward with its own nearly identical copy of the "upload service". I'm trying to fix the bug again at #3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource but @Wim Leers "absolutely, completely, vehemently disagree[s] with this." ;)

I agree we need to fix it in REST, too, hence this issue. :)

Proposed resolution

If fopen() returned FALSE, don't call fclose() on that "file pointer".

Remaining tasks

  1. Upload the patch.
  2. Decide if/how we can write a test for this. *sigh* I don't really want to jump through all the hoops to mock the file system for this. Can't we plainly see that calling fclose(FALSE) is a bad idea, remove that line, and sleep easy if all the rest of the REST tests pass? ;)
  3. Reviews.
  4. RTBC.
  5. Commit.
  6. Re-fix this in JSON:API: #3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE)

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A.

CommentFileSizeAuthor
#2 3036197-2.patch861 bytesdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww created an issue. See original summary.

dww’s picture

Assigned: dww » Unassigned
Status: Active » Needs review
FileSize
861 bytes
dww’s picture

Issue summary: View changes
gabesullice’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +API-First Initiative

Decide if/how we can write a test for this. *sigh* I don't really want to jump through all the hoops to mock the file system for this. Can't we plainly see that calling fclose(FALSE) is a bad idea, remove that line, and sleep easy if all the rest of the REST tests pass? ;)

This seems entirely sensible to me.

dww’s picture

Issue summary: View changes

Opened #3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE) as a child issue to fix this in JSON:API.

If this lands before #2843147: Add JSON:API to core as a stable module that can be fixed in JSON:API contrib. If #2843147 lands first, we can move that issue to the core queue and fix it once this lands (or possibly merge that in and fix both in the same commit).

Ahh, the joys of hoop jumping! ;)
-Derek

alexpott’s picture

Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 04343d7d91 to 8.7.x and 3528551bb1 to 8.6.x. Thanks!

  • alexpott committed 04343d7 on 8.7.x
    Issue #3036197 by dww: REST FileUploadResource::streamUploadData() can...

  • alexpott committed 3528551 on 8.6.x
    Issue #3036197 by dww: REST FileUploadResource::streamUploadData() can...
dww’s picture

Issue summary: View changes

Excellent, thanks! I just unblocked the JSON:API issue.

Cheers,
-Derek

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.