Re: [obs-commits] [openSUSE/open-build-service] 5a83a1: [backend] [api] check the sha256 of file if needed

Hi, On 2017-10-27 05:59:59 -0700, Michael Schroeder wrote:

Branch: refs/heads/master Home: https://github.com/openSUSE/open-build-service Commit: 5a83a195ffbdc1115566ed28b38c8c2665e50ba3 https://github.com/openSUSE/open-build-service/commit/5a83a195ffbdc1115566ed... Author: marco Date: 2017-10-27 (Fri, 27 Oct 2017)

Changed paths: M docs/api/api/api.txt M src/api/app/controllers/source_controller.rb M src/backend/BSSrcrep.pm M src/backend/BSXML.pm M src/backend/bs_srcserver

Log Message: ----------- [backend] [api] check the sha256 of file if needed

This will set $entry->{'hash'} = 'missing' if there is already a file on the server with the same md5sum, but in another project with the same package name.

Hmm isn't the code doing something different here (IMHO, the "wrong" thing)?

@@ -2969,6 +2979,21 @@ sub sourcecommitfilelist { push @missing, $entry; } else { die("duplicate file: $entry->{'name'}\n") if exists $files->{$entry->{'name'}}; + if ($entry->{'hash'}) { + my $fd = gensym; + BSRevision::revopen($orev, $entry->{'name'}, $entry->{'md5'}, $fd); + my $sha256 = Digest::SHA->new(256); + my $hash_to_check = "sha256:" . $sha256->addfile($fd)->hexdigest; + if ($hash_to_check ne $entry->{'hash'}) { + die("SHA missmatch for same md5sum in $packid for file $entry->{'name'} with sum $entry->{'md5'}\n"); + }

(nitpick: "mismatch")

+ } elsif ($cgi->{'withvalidate'}) { + if ((!$ofiles->{$entry->{'name'}} || $ofiles->{$entry->{'name'}} ne $entry->{'md5'}) || + (!$ofiles_expanded->{$entry->{'name'}} || $ofiles_expanded->{$entry->{'name'}} ne $entry->{'md5'})) { + $entry->{'hash'} = 'missing'; + push @missing, $entry;

If I understand this feature correctly, the "ne" should be an "eq", because if $ofiles->{$entry->{'name'}} equals $entry->{'md5'} it might be a potential md5 collision and we should check the sha256. Actually, I do not understand these $ofiles and $ofiles_expanded checks at all. IMHO, we should always set $entry->{'hash'} = 'missing', because if the file exists in the srcrep, there might be a potential md5 collision. That is, it should be something like this if ($entry->{'hash'}) { ... } else { $entry->{'hash'} = 'missing'; push @missing, $entry; } Marcus